[DashboardLayout] Show nested navigation in mini-drawer by apedroferreira · Pull Request #4276 · mui/toolpad

apedroferreira · 2024-10-18T17:35:45Z

Initially was just trying to refactor the sidebar navigation in DashboardLayout to use the same navigation cache and utilities as the PageContainer component.

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-03-13.at.18.08.48.mov

Also improved selected navigation item tests to cover an extra case.

Inspiration:

https://minimals.cc/dashboard

douglaszaltron · 2024-11-08T20:06:08Z

Nice!

…cted-item

mui-bot · 2025-01-16T11:53:16Z

Netlify deploy preview

https://deploy-preview-4276--mui-toolpad-docs.netlify.app/

Generated by 🚫 dangerJS against 145f76b

…cted-item

bharatkashyap

Looks really good! Great addition to the feature-set. Some small comments on code.

bharatkashyap · 2025-01-17T08:55:39Z

packages/toolpad-core/src/DashboardLayout/DashboardSidebarSubNavigation.tsx

          >
            <NavigationListItemButton
-              selected={isSelected && (!navigationItem.children || isMini)}
+              selected={


Might be helpful to add some comments to explain the thinking behind the really long ternaries

bharatkashyap · 2025-01-17T08:57:36Z

packages/toolpad-core/src/DashboardLayout/DashboardSidebarSubNavigation.tsx

-
-        if (isSelected && !selectedItemId) {
-          selectedItemId = navigationItemId;
+        let nestedNavigationCollapseSx: SxProps<Theme> = { display: 'none' };


Seems like this could be a useMemo to prevent having to recalculate on every render

bharatkashyap · 2025-01-17T08:59:17Z

packages/toolpad-core/src/shared/navigation.tsx

    if (isPageItem(item)) {
-      const path = `${base}${item.segment ? `/${item.segment}` : ''}` || '/';
+      const path =
+        `${base.startsWith('/') ? base : `/${base}`}${base && base !== '/' && item.segment ? '/' : ''}${item.segment || ''}` ||


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

bharatkashyap · 2025-01-17T09:06:31Z

packages/toolpad-core/src/shared/navigation.tsx

+        return hasSelectedNavigationChildren(navigation, nestedItem, activePagePath);
+      }
+
+      return activePagePath === getItemPath(navigation, nestedItem);


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

I believe the activePagePath that goes here as a parameter already accounts for pattern matching with the logic in the useActivePage hook. I'll check again just to be sure.

So the behavior should be that pattern matching in children items works, but only under the segments in the parent items. If the parent also has a pattern, that pattern in the parent will probably be ignored in these cases and I think that's probably the way it should work.

bharatkashyap · 2025-01-17T09:09:05Z

packages/toolpad-core/src/DashboardLayout/DashboardSidebarSubNavigation.tsx

+                <Box
+                  sx={{
+                    position: 'fixed',
+                    left: 62,


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)

royhuntley · 2025-02-06T15:00:14Z

Related a little to this item...

If you have a navigation structure that a PAGE item also has children, when the navigation drawer is open (Wide), you cant actually navigate to the page, because clicking on it expands to make the children visible. However, when the navigation drawer is closed (Narrow), you get taken to the page when you click on the item.

{
segment: 'test',
title: 'Test',
children: [ {
segment: 'orders',
title: 'Orders',
icon: ,
},
{
kind: 'divider'
},
{
segment: 'orders',
title: 'Orders',
icon: ,
},

apedroferreira · 2025-02-07T12:31:17Z

Related a little to this item...

If you have a navigation structure that a PAGE item also has children, when the navigation drawer is open (Wide), you cant actually navigate to the page, because clicking on it expands to make the children visible. However, when the navigation drawer is closed (Narrow), you get taken to the page when you click on the item.

{ segment: 'test', title: 'Test', children: [ { segment: 'orders', title: 'Orders', icon: , }, { kind: 'divider' }, { segment: 'orders', title: 'Orders', icon: , },

Hey, I know, that behavior will change once this PR is merged.

…cted-item

apedroferreira · 2025-03-13T18:41:18Z

Didn't want to abandon this old PR so addressed review suggestions + improved mini bar design as the previous one here wasn't great.
Updated the video above with the new mini bar design.

royhuntley · 2025-03-13T21:09:01Z

What needs to happen to get this change released - its good work - the menu bar is so much more functional when minimised!On 13 Mar 2025, at 18:41, Pedro Ferreira ***@***.***> wrote: Didn't want to abandon this old PR so fixed what made sense from the review suggestions + improved mini bar design as the previous one here wasn't great. Updated the video above with the new mini bar design.—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***> apedroferreira left a comment (mui/toolpad#4276) Didn't want to abandon this old PR so fixed what made sense from the review suggestions + improved mini bar design as the previous one here wasn't great. Updated the video above with the new mini bar design. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>

apedroferreira · 2025-03-18T17:10:45Z

I'll merge this one soon if it's good with everyone!

bharatkashyap

Looks good!

DaliborHomola · 2025-03-19T09:02:26Z

I'll merge this one soon if it's good with everyone!

Yes, great

Use navigation utils for sidebar selected item

e3b6f68

apedroferreira added the type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. label Oct 18, 2024

apedroferreira self-assigned this Oct 18, 2024

apedroferreira changed the title ~~Use navigation utils for sidebar selected item~~ [DashboardLayout] Refactor sidebar selected item logic Oct 18, 2024

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 18, 2024

apedroferreira added 6 commits October 22, 2024 17:17

Change props order

6adbbd1

Missing full stop

10670e9

Fix false warnings

998c968

Temporary fix for mini-drawer selected item

c0165b9

Mini-drawer popovers without animations

3d05f7c

Remove Grow component

4a82b51

apedroferreira changed the title ~~[DashboardLayout] Refactor sidebar selected item logic~~ [DashboardLayout] Show nested navigation in mini-drawer Oct 28, 2024

oliviertassinari added component: DashboardLayout labels Nov 8, 2024

apedroferreira added 2 commits November 8, 2024 17:21

Use animated popover for nested menus on hover

228d610

Show tooltips for folder items too

c26bb41

prakhargupta1 removed the component: DashboardLayout label Nov 11, 2024

smol refactor

8d6a960

This comment was marked as resolved.

Sign in to view

Merge remote-tracking branch 'upstream/master' into refactor-nav-sele…

ee59d32

…cted-item

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jan 16, 2025

apedroferreira added 2 commits January 16, 2025 13:59

Add a test and final fixes

9acf943

Merge remote-tracking branch 'upstream/master' into refactor-nav-sele…

98e83dc

…cted-item

apedroferreira marked this pull request as ready for review January 16, 2025 14:03

apedroferreira requested a review from a team January 16, 2025 14:03

bharatkashyap approved these changes Jan 17, 2025

View reviewed changes

Review suggestions + improve mini sidebar look

f4f8a35

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 13, 2025

Remove stuff from testing

17ee76f

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 13, 2025

Merge remote-tracking branch 'upstream/master' into refactor-nav-sele…

145f76b

…cted-item

apedroferreira requested a review from a team March 13, 2025 18:41

apedroferreira moved this from Backlog to In progress in Toolpad public roadmap Mar 17, 2025

douglaszaltron approved these changes Mar 18, 2025

View reviewed changes

bharatkashyap approved these changes Mar 19, 2025

View reviewed changes

apedroferreira merged commit 2bf6a0b into mui:master Mar 19, 2025
15 checks passed

apedroferreira deleted the refactor-nav-selected-item branch March 19, 2025 13:45

github-project-automation bot moved this from In progress to Completed in Toolpad public roadmap Mar 19, 2025

Uh oh!

Conversation

apedroferreira commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

douglaszaltron commented Nov 8, 2024

Uh oh!

This comment was marked as resolved.

mui-bot commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Netlify deploy preview

Uh oh!

bharatkashyap left a comment

Choose a reason for hiding this comment

Uh oh!

bharatkashyap Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

bharatkashyap Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

bharatkashyap Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

bharatkashyap Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

apedroferreira Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatkashyap Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

royhuntley commented Feb 6, 2025

Uh oh!

apedroferreira commented Feb 7, 2025

Uh oh!

apedroferreira commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

royhuntley commented Mar 13, 2025 via email

Uh oh!

apedroferreira commented Mar 18, 2025

Uh oh!

bharatkashyap left a comment

Choose a reason for hiding this comment

Uh oh!

DaliborHomola commented Mar 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

apedroferreira commented Oct 18, 2024 •

edited

Loading

mui-bot commented Jan 16, 2025 •

edited

Loading

apedroferreira Mar 12, 2025 •

edited

Loading

apedroferreira commented Mar 13, 2025 •

edited

Loading