Polaris-react: Navigation rollup always collapses when section heading is clicked

[clauderic]

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 the Navigation.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

[chloerice]

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.

This sounds like a great idea 👌

[clauderic]

Any update on this @chloerice?

[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 Navigation component is sparsely documented and the Navigation.Section and SubNavigationItem subcomponents are completely undocumented. Almost all of what goes on under the hood is in those subcomponents.

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 web nav to see if that discrepancy is an issue with the implementation or the component itself. The examples don't currently have routing set up, and there isn't an example of the admin's existing nav UX with the collapsing items.

I'm still looking into the fix for the issue you outlined with the SubNavigationItem, but I won't be able to get back to it until the end of the week.

[clauderic]

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

[chloerice]

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:

• Discounts (which navigates to the url of its first subNavigationItem and auto-closes the nav so Automatic discounts is impossible to get to on small screens)
• Products (which works as expected)
• Online store (which does not lock you into a single view like Discounts, but does auto-close on clicking the section item, requiring you to re-open the nav in order to select a subNavigationItem)

[clauderic]

Hey @chloerice, you have to use the rollup prop of Navigation.Section in order to be able to reproduce the issue, see this CodeSandbox reproducing the issue: https://codesandbox.io/s/8xym875zq8

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.

[asalisbury]

👋 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.

[beefchimi]

Duplicate of: https://github.com/Shopify/polaris-react/issues/738

This is the more detailed issue however... so I'm going to close 738

[tmlayton]

@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:

https://github.com/Shopify/polaris-react/blob/6dcef797896856cdeff9bd92cb26fd4a433549db/src/components/Navigation/components/Section/Section.tsx#L179-L181

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.

[tmlayton]

Fix ready for review: https://github.com/Shopify/polaris-react/pull/1417