feat(core): update nav to work better on mobile and when collapsed

[DominikB2014]

Updates the nav according to the following mock, the sidebar currently doesn't do anything like the last settings example, so this case hasn't been considered for this version, but we can add it in the future
It would be super helpful for any reviewers to play around with it!

[gggritso]

Starting to look really solid! A few questions/comments about the UI/code

• if an accordion is open, clicking the button again should close it, rather than re-run the opening animation
• the hover tooltip on a collapsed sidebar button doesn't need to show up if that accordion is open
• the menu could use a bit more width so "App Starts" isn't cut off

Did you have a change to try this out on a physical phone? A lot of weird stuff pops up there!

As for the code, everything looks reasonable, but a few questions:

• you're managing the open state with a context around every accordion. Why every accordion, since only one is open at a time? Also, doesn't this clash with the [expanded, setExpanded] state that's kept in localStorage?
• the isFloatingSidebar prop is pretty confusing. It gets passed far down, but its name is strange, it's implying that the item is a floating sidebar? Maybe something like isFloating is clearer. Ideally, though, maybe the SidebarAccordion would set that prop on its children, or that state could be inferred through CSS? That might be complicated
• IMO getBoundingClientRect is fine but since those elements are children of the accordion, it should in theory be possible to position them relatively, without having to use JS! Maybe not, though

[DominikB2014]

@gggritso I'm going to go through all those bugs! The reopening subitem thing is because the "onclickoutside" is firing which closes it, and then the "onMainItemClick" fires which reopens it! I can probably do something where onClickOutside ignores clicks on the "mainItem" in the sidebar. This was mb, i quickly added the onClickOutside last minute

As for the code, everything looks reasonable, but a few questions:

• you're managing the open state with a context around every accordion. Why every accordion, since only one is open at a time?

So the context is actually around the entire sidebar, that way each accordion can share state of what's currently open. This would only be used in the state where the sub items should be floating (mobile or sidebar collapsed), I suppose technically you can get away from omitting this shared state, and can manage it locally, is this what you mean? When clicking from one set of sub items to the next, we would be relying on the "onClickOutside" to close the current sub items, and then the onclick of the next accoridion would open the new sub menus. This to me sounds less intuitive and less robust, theoretically its possible that both accordions have sub menus open with this approach, sharing one state of what's open accross accordions would only ever allow one set of sub items to open at once.

Also, doesn't this clash with the [expanded, setExpanded] state that's kept in localStorage?

It shouldn't clash at all, the accoridan expanded/setExpanded local storage state is not applicable to the "floating" sub items, only to the original expand/collapse sidebar behaviour. This makes total sense imo, because when its not in a "floating" state, we are able to open multiple submenu items at once, so we can't rely on the global state that tells us what's open. And we want to be able to refresh and the same sub-items are open (which is not applicable in the floating state)

• the isFloatingSidebar prop is pretty confusing. It gets passed far down, but its name is strange, it's implying that the item is a floating sidebar? Maybe something like isFloating is clearer. Ideally, though, maybe the SidebarAccordion would set that prop on its children, or that state could be inferred through CSS? That might be complicated

Yeah that's totally fair! What its trying to say, is that the subitem should be "floating" when expanded, what if we did "shouldSubItemFloat"? But I like your idea of setting via the accoridon, sounds a little more bullet proof, will dig into that!

• IMO getBoundingClientRect is fine but since those elements are children of the accordion, it should in theory be possible to position them relatively, without having to use JS! Maybe not, though

Yeah this sounds possible actually, and I don't think its too complicated, I'll give it a go!

[gggritso]

Thanks for explaining everything! I get what you're doing now, and I like this approach 👍🏻 This time leaving some actual more specific line comments

Looking good, very close now

[codecov]

Bundle Report

Changes will increase total bundle size by 49.07kB ⬆️

Bundle name	Size	Change
sentry-webpack-bundle-array-push	26.24MB	49.07kB ⬆️

[vuluongj20]

Let's use the Overlay component in place of SidebarDropdownMenu

[DominikB2014]

@vuluongj20 updated to use the Overlay Component!

[vuluongj20]

@DominikB2014 okay last one I promise 😄 Let's remove some of the padding around the menu items?

The menu should look like this:

[DominikB2014]

@vuluongj20 Made the changes to match the padding!