Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] When clicking on the left drawer, ancre doesn't stay on the click #25560

Closed
paulo60pg opened this issue Mar 30, 2021 · 8 comments · Fixed by #25619
Closed

[docs] When clicking on the left drawer, ancre doesn't stay on the click #25560

paulo60pg opened this issue Mar 30, 2021 · 8 comments · Fixed by #25619
Labels
bug 🐛 Something doesn't work component: drawer This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. scope: docs-infra Specific to the docs-infra product

Comments

@paulo60pg
Copy link

The drawer seems to always go back to the top after each click.

I would expect the same behavior as V4 documentation.

@paulo60pg paulo60pg added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Mar 30, 2021
@oliviertassinari oliviertassinari added component: drawer This is the name of the generic UI component, not the React module! status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Mar 30, 2021
@oliviertassinari

This comment has been minimized.

@paulo60pg
Copy link
Author

The left drawer doesn't stay where the click is made. Which creates confusion on where we are in the doc.

Please find a video below :

material-ui.mov

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation and removed status: waiting for author Issue with insufficient information labels Mar 30, 2021
@oliviertassinari
Copy link
Member

It looks like a regression from #24418

@oliviertassinari oliviertassinari added the design: material This is about Material Design, please involve a visual or UX designer in the process label Mar 31, 2021
@oliviertassinari oliviertassinari changed the title V5 documentation : When clicking on the left drawer, ancre doesn't stay on the click [docs] When clicking on the left drawer, ancre doesn't stay on the click Mar 31, 2021
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work and removed design: material This is about Material Design, please involve a visual or UX designer in the process labels Mar 31, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 31, 2021

It looks like a regression from #24418

Actually no. I could pinpoint the origin. It's about #25416. It's a breaking change between React 16 and React 17, introduced in facebook/react#17925. @dtassone one example of why the sooner we run the data grid in React 17 the better :).

@paulo60pg we can restore the previous behavior with a simple change:

diff --git a/docs/src/modules/components/AppNavDrawer.js b/docs/src/modules/components/AppNavDrawer.js
index 267ab9512e..da51364794 100644
--- a/docs/src/modules/components/AppNavDrawer.js
+++ b/docs/src/modules/components/AppNavDrawer.js
@@ -22,7 +22,7 @@ function PersistScroll(props) {
   const { children, enabled } = props;
   const rootRef = React.useRef();

-  React.useEffect(() => {
+  React.useLayoutEffect(() => {
     const parent = rootRef.current ? rootRef.current.parentElement : null;
     const activeElement = parent.querySelector('.app-drawer-active');

Would you like to work on a pull request? :)

@oliviertassinari
Copy link
Member

On a different note. The scroll experience on mobile is horrible. it drove me crazy for the last 2 years. I would propose we fix it with:

diff --git a/docs/src/modules/components/AppNavDrawer.js b/docs/src/modules/components/AppNavDrawer.js
index 267ab9512e..8cf065f1af 100644
--- a/docs/src/modules/components/AppNavDrawer.js
+++ b/docs/src/modules/components/AppNavDrawer.js
@@ -22,7 +22,7 @@ function PersistScroll(props) {
   const { children, enabled } = props;
   const rootRef = React.useRef();

-  React.useEffect(() => {
+  React.useLayoutEffect(() => {
     const parent = rootRef.current ? rootRef.current.parentElement : null;
     const activeElement = parent.querySelector('.app-drawer-active');

@@ -30,13 +30,12 @@ function PersistScroll(props) {
       return undefined;
     }

+    parent.scrollTop = savedScrollTop;
+
     const activeBox = activeElement.getBoundingClientRect();

-    if (savedScrollTop === null || activeBox.top - savedScrollTop < 0) {
-      // Center the selected item in the list container.
-      activeElement.scrollIntoView();
-    } else {
-      parent.scrollTop = savedScrollTop;
+    if (activeBox.top < 0 || activeBox.top > window.innerHeight) {
+      parent.scrollTop += activeBox.top - 8 - 32;
     }

     return () => {
@@ -200,10 +199,10 @@ function AppNavDrawer(props) {
             keepMounted: true,
           }}
         >
-          {drawer}
+          <PersistScroll enabled={mobileOpen}>{drawer}</PersistScroll>
         </SwipeableDrawer>
       ) : null}
-      {disablePermanent ? null : (
+      {disablePermanent || mobile ? null : (
         <Hidden lgDown implementation="css">
           <Drawer
             classes={{
@@ -212,7 +211,7 @@ function AppNavDrawer(props) {
             variant="permanent"
             open
           >
-            <PersistScroll enabled={!mobile}>{drawer}</PersistScroll>
+            <PersistScroll enabled>{drawer}</PersistScroll>
           </Drawer>
         </Hidden>
       )}

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Mar 31, 2021
@misaka3
Copy link
Contributor

misaka3 commented Apr 4, 2021

@oliviertassinari
Hi, I want to work this issue.Is it okay?

@oliviertassinari
Copy link
Member

@misaka3 Feel free to :)

@misaka3
Copy link
Contributor

misaka3 commented Apr 4, 2021

@oliviertassinari Thanks!

notsidney added a commit to rowyio/rowy that referenced this issue Jul 12, 2021
* develop:
  add badge to UserMenu when update is available
  migrate all links to WIKI_LINKS
  add UpdateChecker
  default signInOptions to google if signInOptions doc is missing
  fix TableSettings multiSelects
  TextEditor: fix values not saving in react 17 (thanks @oliviertassinari ) mui/material-ui#25560 (comment) facebook/react#17925
  expose dataset location option for BQ spark
@oliviertassinari oliviertassinari added the scope: docs-infra Specific to the docs-infra product label Dec 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: drawer This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants