Skip to content
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.

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

fix: render divider and header as li in menu #4763

Conversation

rkristelijn
Copy link
Contributor

@rkristelijn rkristelijn commented Mar 14, 2025

  • 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 fix: render divider as li in menu #4762 ".
  • I've added a visual demonstration in the form of a screenshot or video.

Closes #4759

Copy link
Member

@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.

Hi, you're right, thanks a lot for finding these issues and working on the fixes!
All good, just a small change in the order of props to be consistent.

@mui-bot
Copy link

mui-bot commented Mar 19, 2025

Netlify deploy preview

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

Generated by 🚫 dangerJS against d96a68d

@apedroferreira
Copy link
Member

apedroferreira commented Mar 19, 2025

There's a failing CI check, sorry, can you please run pnpm prettier:all?
I also just added a commit to rebase on the master branch so be sure to pull that commit before adding changes.
Other than that hopefully everything should be passing! Thanks again!

… github.com:rkristelijn/toolpad into feature/render-divider-as-li-in-menu-and-header-too
@rkristelijn
Copy link
Contributor Author

rkristelijn commented Mar 19, 2025

I've ran npx pnpm prettier:all

Weirdly enough there is no pnpm dependency, so i ran against the latest which is 9.12.3.

❯ npm ls pnpm
[email protected] /path/to/toolpad
└── (empty)

❯ npx pnpm --version
9.12.3

I would recommend to either:

  1. add to README.md to use corepack enable && corepack prepare pnpm --activate
  2. add in .npmrc: [email protected] or other version
  3. (my preference) add the right version of pnpm as a dev dependency that can be used via npx (first uses local installs, then globals)

@apedroferreira apedroferreira merged commit ed2360c into mui:master Mar 19, 2025
15 checks passed
@apedroferreira
Copy link
Member

I would recommend to either:

  1. add to README.md to use corepack enable && corepack prepare pnpm --activate
  2. add in .npmrc: [email protected] or other version
  3. (my preference) add the right version of pnpm as a dev dependency that can be used via npx (first uses local installs, then globals)

Yes, looks like we could improve the documentation about suggestion 1 at least. Feel free to even submit a PR with that or I can do it soon!

Other than that we do have "packageManager": "[email protected]" in the root package.json, and it doesn't seem like we're following number 3 organization-wide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: toolpad-core Abbreviated to "core"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DashboardLayout renders inaccessible menu for divider
5 participants