Skip to content

[DashboardLayout] Highlight and text alignment for mini #4844

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 17, 2025
Merged

[DashboardLayout] Highlight and text alignment for mini #4844

merged 3 commits into from
Apr 17, 2025

Conversation

bharatkashyap
Copy link
Collaborator

@bharatkashyap bharatkashyap commented Apr 14, 2025 •
edited
Loading

Before After
Screen.Recording.2025-04-16.at.2.08.56.PM.mov

@bharatkashyap bharatkashyap added the design: ui Visual design. label Apr 14, 2025
@mui-bot
Copy link

mui-bot commented Apr 14, 2025 •
edited
Loading

Netlify deploy preview

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

Generated by 🚫 dangerJS against e360c1c

apedroferreira
apedroferreira requested changes Apr 16, 2025
View reviewed changes
packages/toolpad-core/src/DashboardLayout/DashboardSidebarSubNavigation.tsx Outdated
@@ -287,9 +287,9 @@ function DashboardSidebarSubNavigation({
sx={{
position: 'absolute',
bottom: -18,
left: '50%',
left: '45%',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't change this, this is what is centering the text and it goes together with transform: 'translateX(-50%)' below.
And also the font size shouldn't be what is causing the issue and it shouldn't be changed unless necessary.

Can we not just set a minimum width somewhere so that the scrollbar doesn't make things narrower?
The current changes are only trying to disguise the problem but the content is not centered.
I can take a deeper look if it helps.

@bharatkashyap
Copy link
Collaborator Author

bharatkashyap commented Apr 16, 2025 •
edited
Loading

@apedroferreira I've changed the solution to use the MINI_DRAWER_WIDTH for the ul and passing scrollbar-gutter: stable to the nav when isMini is set, everything being the same in all other modes.

Screen.Recording.2025-04-16.at.2.08.56.PM.mov

apedroferreira
apedroferreira approved these changes Apr 17, 2025
View reviewed changes
Copy link
Collaborator

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a lot better, thanks a lot for the changes!

@bharatkashyap bharatkashyap merged commit 039fe71 into mui:master Apr 17, 2025
15 checks passed
@apedroferreira
Copy link
Collaborator

apedroferreira commented Apr 17, 2025 •
edited
Loading

Looks like there's a horizontal scrollbar when the sidebar is in mini mode now: https://deploy-preview-4809--mui-toolpad-docs.netlify.app/toolpad/core/react-dashboard-layout

Maybe it'll be fixed if we remove a few pixels from the width?

@bharatkashyap
Copy link
Collaborator Author

Looks like there's a horizontal scrollbar when the sidebar is in mini mode now: https://deploy-preview-4809--mui-toolpad-docs.netlify.app/toolpad/core/react-dashboard-layout

Maybe it'll be fixed if we remove a few pixels from the width?

We could do that or hide scrollbars on the container nav of the list, which approach do you think makes more sense? Either make sense to me, although the overflow-x: hidden on the container nav seems to be more robust since we never want horizontal scrollbars to appear in the sidebar

@apedroferreira
Copy link
Collaborator

Looks like there's a horizontal scrollbar when the sidebar is in mini mode now: https://deploy-preview-4809--mui-toolpad-docs.netlify.app/toolpad/core/react-dashboard-layout
Maybe it'll be fixed if we remove a few pixels from the width?

We could do that or hide scrollbars on the container nav of the list, which approach do you think makes more sense? Either make sense to me, although the overflow-x: hidden on the container nav seems to be more robust since we never want horizontal scrollbars to appear in the sidebar

I would try to figure out why there is an overflow now - if it's related with having set a width in the items, I would try to fix something related to that. I don't mind taking a look soon.

@bharatkashyap
Copy link
Collaborator Author

Looks like there's a horizontal scrollbar when the sidebar is in mini mode now: https://deploy-preview-4809--mui-toolpad-docs.netlify.app/toolpad/core/react-dashboard-layout
Maybe it'll be fixed if we remove a few pixels from the width?

We could do that or hide scrollbars on the container nav of the list, which approach do you think makes more sense? Either make sense to me, although the overflow-x: hidden on the container nav seems to be more robust since we never want horizontal scrollbars to appear in the sidebar

I would try to figure out why there is an overflow now - if it's related with having set a width in the items, I would try to fix something related to that. I don't mind taking a look soon.

The scrollbar is due to providing an explicit width to the list now - reducing the width makes the scrollbar go away but I haven't tested this fix rigorously over all device sizes. I can open a PR with that fix and you can take it forward/review it

@bharatkashyap bharatkashyap mentioned this pull request Apr 18, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apedroferreira apedroferreira apedroferreira approved these changes

Assignees
No one assigned
Labels
design: ui Visual design.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sidebar selected item styling misalignment when scrollbar appears in collapsed state
3 participants
@bharatkashyap @mui-bot @apedroferreira