[DashboardLayout] Show nested navigation in mini-drawer #4276
Conversation
apedroferreira
changed the title
github-actions
bot
added
the
PR: out-of-date
apedroferreira
changed the title
oliviertassinari
added
component: DashboardLayout
component: layout
|
Nice! |
This comment was marked as resolved.
This comment was marked as resolved.
github-actions
bot
removed
the
PR: out-of-date
Netlify deploy preview |
| sx={{ | ||
| py: 0, | ||
| px: 1, | ||
| overflowX: 'hidden', | ||
| }} | ||
| > | ||
| <NavigationListItemButton | ||
| selected={isSelected && (!navigationItem.children || isMini)} | ||
| selected={ |
There was a problem hiding this comment.
Might be helpful to add some comments to explain the thinking behind the really long ternaries
|
|
||
| if (isSelected && !selectedItemId) { | ||
| selectedItemId = navigationItemId; | ||
| let nestedNavigationCollapseSx: SxProps<Theme> = { display: 'none' }; |
There was a problem hiding this comment.
Seems like this could be a useMemo to prevent having to recalculate on every render
| @@ -67,7 +25,9 @@ function buildItemToPathMap(navigation: Navigation): Map<NavigationPageItem, str | |||
|
|
|||
| const visit = (item: NavigationItem, base: string) => { | |||
| if (isPageItem(item)) { | |||
| const path = `${base}${item.segment ? `/${item.segment}` : ''}` || '/'; | |||
| const path = | |||
| `${base.startsWith('/') ? base : `/${base}`}${base && base !== '/' && item.segment ? '/' : ''}${item.segment || ''}` || | |||
There was a problem hiding this comment.
Similar comment about comments being helpful on really long ternaries. Even though this one seems relatively straightforward, a comment helps bolt down the original intent instead of having to guess/derive
| return hasSelectedNavigationChildren(navigation, nestedItem, activePagePath); | ||
| } | ||
|
|
||
| return activePagePath === getItemPath(navigation, nestedItem); |
There was a problem hiding this comment.
To be sure, does this function work in cases where the nested navigation items are supplied via regex patterns? I might be missing something, but seems like the earlier function was checking for a navigationItem.pattern case and testing the path against the regex pattern in case it existed but I can't seem to find an analogous case here
| <Box | ||
| sx={{ | ||
| position: 'fixed', | ||
| left: 62, |
There was a problem hiding this comment.
Is there no avoidable way to hard code the left and pl values here? I could be wrong but this might create a future bug if we make the size of the mini sidebar configurable - perhaps we can link these values to that constant (if indeed they derive from it)
Initially was just trying to refactor the sidebar navigation in
DashboardLayoutto use the same navigation cache and utilities as thePageContainercomponent.But this required adding nested navigation to the sidebar for consistency with the warnings we show when there are duplicated navigation items, because the navigation links should match in all variants of the sidebar:
Screen.Recording.2025-01-16.at.13.56.19.mov
Also improved selected navigation item tests to cover an extra case.
Inspiration: