DOP-4883: Menu not closing after selection on Mobile #1254
Conversation
✅ Deploy Preview for mongodb-snooty ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Nice work, thanks for picking this up! I think there are a couple things that need a bit of tweaking:
- I actually think the previous behavior you showed in Slack was what we were looking for, where clicking on the text of the link opens the page and hides the Sidenav (even if there are children)
- I think because we are not using the
setIsOpen(!isOpen)anymore, if you click on an opened caret (on Desktop) it doesn't close anymore (for example, navigate to a page with children and then click on the sidenav item (text or twisty) for the current page you're on - in prod it closes back up). You will also see one of the tests failing because of this! If we can preserve the current behavior of the dropdown so that you can open and close the side nav item even when you're on the page that would be great!
|
Thanks for the comments! Both things should be fixed by the 2 latest commits. Please review again and let me know if you see more issues :) |
src/components/Sidenav/TOCNode.js
Outdated
| @@ -82,6 +82,11 @@ const TOCNode = ({ activeSection, handleClick, level = BASE_NODE_LEVEL, node, pa | |||
| setIsOpen(!isOpen); | |||
| }; | |||
|
|
|||
| const clickSideNav = (handleClick) => { | |||
| setIsOpen(!isOpen); | |||
There was a problem hiding this comment.
oh no, now the carets/"twisties" are being treated like text in mobile sidenav in that if you click them they open the page and hide the sidenav, instead of just opening the caret to see the children dropdown. I think setting the state here is probably interfering with onCaretClick. Lmk if you want to discuss this more!
There was a problem hiding this comment.
agreed! i think we can leave the behaviors as is, and actually add a listener on location change that closes the mobile menu. ie something like this pattern
There was a problem hiding this comment.
@seungpark , Im a little confused on how the link you sent is suppose to fix my issue. Sould I not be using the handleclick() anymore that I passed in from sidenav.js ? And rewrite that function in the tocnode.js file.
There was a problem hiding this comment.
i think this is more of a set state to close menu on location change than on sidenav button click (which would trigger a location change). lmk if that makes sense!
we can leave the behaviors as is
I meant as to leave the current click handlers, and add a new effect in the component to listen for location changes
There was a problem hiding this comment.
@seungpark and @mayaraman19 . Thanks for the advice I made a change that I think changes this problem. Please let me know if you see anything else.
src/components/Sidenav/TOCNode.js
Outdated
| @@ -82,6 +82,11 @@ const TOCNode = ({ activeSection, handleClick, level = BASE_NODE_LEVEL, node, pa | |||
| setIsOpen(!isOpen); | |||
| }; | |||
|
|
|||
| const clickSideNav = (handleClick) => { | |||
| setIsOpen(!isOpen); | |||
There was a problem hiding this comment.
agreed! i think we can leave the behaviors as is, and actually add a listener on location change that closes the mobile menu. ie something like this pattern
package-lock.json
Outdated
There was a problem hiding this comment.
not sure why we have changes here! there shouldnt be any diff if we merge master into your branch
src/components/Sidenav/Sidenav.js
Outdated
| @@ -203,6 +205,11 @@ const Sidenav = ({ chapters, guides, page, pageTitle, repoBranches, slug, eol }) | |||
| setHideMobile(true); | |||
| }, [setHideMobile]); | |||
|
|
|||
| // close navigation panel on mobile screen, but leaves open if they click on a twsity | |||
|
discussed offline of the package diff. this was failing in main branch so this actually will resolve this ! |
Stories/Links:
DOP-4883
Current Behavior:
Current Behavior
Staging Links:
Stagging Behavior
Notes:
README updates