Skip to content

Conversation

vikasgurjar
Copy link
Contributor

@vikasgurjar vikasgurjar commented Dec 6, 2024

Closes #4406

  • I've read and followed the contributing guide on how to create great pull requests.
  • I've updated the relevant documentation for any new or updated feature.
  • I've linked relevant GitHub issue with "Closes #".
  • I've added a visual demonstration in the form of a screenshot or video.
Screen.Recording.2024-12-06.at.3.00.24.PM.mov

@vikasgurjar
Copy link
Contributor Author

@Janpot @bharatkashyap from previous convo here

edit: actually, it looks like the data grid has the convention of "xyzOpen", "onXyzOpen" and "onXyzClose". Perhaps that's the closest in spirit?

@Janpot Are you suggesting to have two different callbacks for open and close events? onSidebarOpen and onSidebarClosed?

When changing any of the demos and prop types, make sure to run pnpm docs:typescript:formatted to format TS demos and pnpm:proptypes to autogenerate prop type changes
I ran that command before raising the PR. What I have noticed is this lines props are wrongly generated. Correct proptype should be Proptype.func.required I think.

@Janpot
Copy link
Member

Janpot commented Dec 6, 2024

Yes, I'm suggesting to make this similar to how we do this in other products of the MUI ecosystem. for example menuOpen and friends in the datagrid.

So something like navigationMenuOpen, onNavigationMenuOpen, onNavigationMenuClose. I have a slight preference of naming it semantically (navigation menu) over naming it according to where it's laid out (side bar). This leaves the option open to introduce other variants some day. Not sure how the team feels about this @bharatkashyap @apedroferreira did we already name it this way in other places? If so maybe it makes more sense to stay consistent?

please wait a moment with implementing this to allow us get to a consensus.

@apedroferreira
Copy link
Collaborator

apedroferreira commented Dec 6, 2024

Yes, I'm suggesting to make this similar to how we do this in other products of the MUI ecosystem. for example menuOpen and friends in the datagrid.

So something like navigationMenuOpen, onNavigationMenuOpen, onNavigationMenuClose. I have a slight preference of naming it semantically (navigation menu) over naming it according to where it's laid out (side bar). This leaves the option open to introduce other variants some day. Not sure how the team feels about this @bharatkashyap @apedroferreira did we already name it this way in other places? If so maybe it makes more sense to stay consistent?

please wait a moment with implementing this to allow us get to a consensus.

I've been calling it sidebar when it's purely referred to inside the DashboardLayout component (some props already follow this logic), but "navigationMenu" would also be fine and is more semantic in terms of the global app - we could make an effort to gradually rename things in this direction.

About referring to the property as "open", it does make more sense than referring to it as "collapsed" now that we're making it a controlled prop... So I agree with your suggestions.

@Janpot Janpot added type: new feature Expand the scope of the product to solve a new problem. scope: toolpad-core Abbreviated to "core" labels Dec 6, 2024
@vikasgurjar
Copy link
Contributor Author

@Janpot @apedroferreira I have made changes as per the above suggestions, please review them when possible

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Dec 11, 2024
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.

Thanks for the effort so far, it's looking good but I have many important suggestions!

* @default false
*/
hideNavigation?: boolean;
/** A prop that controls the collapsed state of the sidebar.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/** A prop that controls the collapsed state of the sidebar.
/** If `true`, the navigation menu is expanded.
--
<br class="Apple-interchange-newline">

*/
navigationMenuOpen?: boolean;
/**
* Callback function to be executed on navigation menu state changes to open
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Callback function to be executed on navigation menu state changes to open
* Callback fired when navigation menu is expanded.

*/
onNavigationMenuOpen?: () => void;
/**
* Callback function to be executed on navigation menu state changes to closed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Callback function to be executed on navigation menu state changes to closed
* Callback fired when navigation menu is collapsed.

React.useEffect(() => {
if (typeof navigationMenuOpen === 'boolean') {
setIsNavigationExpanded(navigationMenuOpen);
if (navigationMenuOpen) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When navigationMenuOpen is a boolean, we cannot use the current isDesktopNavigationExpanded or isMobileNavigationExpanded states the way they're currently being used in this component.

So besides this effect, we need to make it so that all checks that involve isDesktopNavigationExpanded or isMobileNavigationExpanded check navigationMenuOpen in those cases instead.

And every current call to setIsDesktopNavigationExpanded or setIsMobileNavigationExpanded can be ignored when navigationMenuOpen is a boolean, but the appropriate onNavigationMenuOpen or onNavigationMenuClose should be called instead.

onNavigationMenuOpen and onNavigationMenuClose should also still be called when navigationMenuOpen is undefined, however, so that they can be used for other cases.

Your current approach requires an extra render for the effect to run, which should not be necessary, and the navigationMenuOpen value can become out of sync with the UI when the internal state is changed.

Hopefully this makes sense and points you in the right direction!

expect(screen.getByText('Hello world')).toBeTruthy();
});

test('renders the sidebar in collapsed state when navigationMenuOpen is false', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for adding tests, they look good!


{{"demo": "DashboardLayoutSidebarHidden.js", "height": 400, "iframe": true}}

### Toggle sidebar
Copy link
Collaborator

@apedroferreira apedroferreira Dec 11, 2024

Choose a reason for hiding this comment

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

This section needs to be a bit more complete as in referring to the whole concept of using "controlled state" for the navigation menu, as well as what kind of use case that could serve.

We also need to document the new callbacks, and how they could be used together with navigationMenuOpen.

Anyway, I can take care of the documentation part when the functionality is done unless you really want to tackle it!

<DashboardLayout navigationMenuOpen={navigationMenuOpen}>
<DemoPageContent
pathname={pathname}
toggleSidebar={() => toggleSidebar(!navigationMenuOpen)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also show the typical usage for the new callbacks in this example, which would change the navigationMenuOpen state accordingly.

const { window } = props;

const [pathname, setPathname] = React.useState('/dashboard');
const [navigationMenuOpen, toggleSidebar] = React.useState<boolean>(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const [navigationMenuOpen, toggleSidebar] = React.useState<boolean>(true);
const [navigationMenuOpen, setNavigationMenuOpen] = React.useState<boolean>(true);

@amortoni
Copy link

Hi team,

Just wondering if there's any update on this PR. It seems like a straightforward and useful addition, and I noticed it's been open since December 2024 with no major changes requested.

Is there anything blocking this from moving forward? We're interested in this feature and would love to see it merged soon.

Thanks for your work on the project!

@apedroferreira
Copy link
Collaborator

Hi team,

Just wondering if there's any update on this PR. It seems like a straightforward and useful addition, and I noticed it's been open since December 2024 with no major changes requested.

Is there anything blocking this from moving forward? We're interested in this feature and would love to see it merged soon.

Thanks for your work on the project!

Toolpad Core won't be actively maintained in the short term so we're not prioritizing this feature right now unfortunately...

We will however publish a template soon in the Material UI docs with more customizable equivalent versions of these components, including separate components for the sidebar and header with the navigation menu expanded state being controlled from the outside and passed to them as props.

@bharatkashyap bharatkashyap changed the title add prop sidebarCollapsed and onToggleSidebar [DashboardLayout] Add prop sidebarCollapsed and onToggleSidebar Jul 12, 2025
@pongells
Copy link

Toolpad Core won't be actively maintained in the short term so we're not prioritizing this feature right now unfortunately...

this is sad news :(
toolpad core seemed really promising, just missing few things like this PR and a tiny bit more customisation.

We will however publish a template soon in the Material UI docs with more customizable equivalent versions of these components, including separate components for the sidebar and header with the navigation menu expanded state being controlled from the outside and passed to them as props.

what do you mean by template? an example using shared components replacing the toolpad core features?

@prakhargupta1
Copy link
Member

what do you mean by template? an example using shared components replacing the toolpad core features?

Yes, a dashboard example built using Material UI core components, so that there is no dependency on Toolpad. Here is a PR for this: mui/material-ui#46376

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged. scope: toolpad-core Abbreviated to "core" type: new feature Expand the scope of the product to solve a new problem.
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

Add props to control sidebar's collapsed state in DashboardLayout component
6 participants