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
Navigation rollup always collapses when section heading is clicked #1004
Comments
This sounds like a great idea 👌 |
Any update on this @chloerice? |
Hey @clauderic, I'm currently helping another team ship a project and didn't mean to let this fall through the cracks. Thanks for the ping. The rollup logic you mentioned handles the optional section rollup, which collapses the section to a set number of items, not the entire sidebar collapse on small screen. (See the "Navigation with section rollup" example ). A big problem here is that the public API of the While digging into this yesterday, I also discovered that there's a few other inconsistencies in the UX between small and large screen. For example, open nav sections don't collapse as other nav sections are opened in small screen, but only a single collapsible nav section at a time is open in large screen (the opposite of what you'd expect if there were to be differences, since small screens have less room...). I still need to dig into the I'm still looking into the fix for the issue you outlined with the |
Hey @chloerice, just following up on this, have you had a chance to get back to this issue? It’s a really annoying issue for our merchants on mobile |
Hey @clauderic, this issue has been my main focus this week, but I've been unable to reproduce it in my reduced test case. There's a lot of inconsistencies in how different sections of the nav work, so I'm moving my focus to the implementations so I can find what the difference is between:
|
Hey @chloerice, you have to use the Is there any way this issue can be prioritized? It's a really annoying/confusing bug that's affecting the experience of users on mobile and with smaller viewports trying to access the Online Store channel. |
👋 Is there anything we can do to help resolve this issue? This should be considered high severity, as it affects ALL mobile users who need to access Shopify's largest sales channel. It's not clear this has been recognized, given how long this ticket has been open for. |
Duplicate of: #738 This is the more detailed issue however... so I'm going to close |
@asalisbury reached out to me on Wed, 5/1 while I was ATC but I did not have a chance to follow up as I was away the rest of the week. As @clauderic points out, the culprit is: We do not want this state to change if the top level nav item is being viewed on mobile and has children. It should simply open to reveal its children. I will follow up with @chloerice today to see if she has a work in progress, otherwise I can pick this up. |
Fix ready for review: #1417 |
Issue summary
The Navigation rollup always collapses
onClick
, and there is no way to prevent that from happening due to the fact that the state ownership to decide whether the rollup should be expanded or collapsed is nested within theNavigation.Section
component (https://github.com/Shopify/polaris-react/blob/master/src/components/Navigation/components/Section/Section.tsx#L181-L183)This issue is blocking fixing https://github.com/Shopify/web/pull/10471 in
shopify/web
.Expected behavior
There should either be a way to prevent the default behaviour of collapsing the rollup onClick (for instance, by returning false in the onClick handler), or the state ownership should be lifted out of the Navigation.Section component and passed down as props instead so that the consumer can decide when to collapse / expand the rollup.
The larger issue is that the Navigation is dismissed when a rollup section header is clicked on mobile. Instead, the navigation should remain open and the rollup disclosure should open/close. Perhaps this should be fixed directly in Polaris as opposed to conditionally passing the
onNavDismiss
handler in the consumer (web
)cc @chloerice
The text was updated successfully, but these errors were encountered: