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

[docs][joy] Build TS versions for Menu component demos #36383

Merged
merged 3 commits into from
Mar 16, 2023

Conversation

varunmulay22
Copy link
Contributor

Menu Part of #36367

@mui-bot
Copy link

mui-bot commented Mar 1, 2023

Netlify deploy preview

https://deploy-preview-36383--material-ui.netlify.app/

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against ae54c6e

Copy link
Member

@hbjORbj hbjORbj left a comment

Choose a reason for hiding this comment

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

This PR includes changes to Typography demos. Please discard them.

@hbjORbj hbjORbj added docs Improvements or additions to the documentation component: menu This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy labels Mar 1, 2023
@hbjORbj hbjORbj changed the title Menu tsx demos [docs][joy] Build TS versions for Menu component demos Mar 1, 2023
@hbjORbj
Copy link
Member

hbjORbj commented Mar 1, 2023

Your branch includes commits that create typography TS demos and deletes them. You shouldn't have both commits in the first place. Please reset your branch to the latest master and only push commits related to Menu component demos.

@varunmulay22 varunmulay22 marked this pull request as draft March 3, 2023 06:53
@varunmulay22 varunmulay22 reopened this Mar 3, 2023
@varunmulay22 varunmulay22 marked this pull request as ready for review March 3, 2023 08:40
@@ -98,15 +97,21 @@ function MenuButton({ children, menu, open, onOpen, onLeaveMenu, label, ...props
placement: 'right-start',
sx: {
width: 288,
[`& .${menuClasses.listbox}`]: {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be deleted. You can add these to the matching TS demo and run yarn docs:typescript:formatted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

problem is menu classes does not have the listbox property

const menuClasses: MenuClasses = generateUtilityClasses('MuiMenu', [ 'root', 'expanded', 'colorPrimary', 'colorNeutral', 'colorDanger', 'colorInfo', 'colorSuccess', 'colorWarning', 'colorContext', 'variantPlain', 'variantOutlined', 'variantSoft', 'variantSolid', 'sizeSm', 'sizeMd', 'sizeLg', ]);

Copy link
Member

Choose a reason for hiding this comment

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

You are right. I opened a new PR to address this issue. After I get a review from another team member on the PR, I will approve this one and merge it. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

This issue is now fixed in #36520

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 6, 2023
Copy link
Member

@hbjORbj hbjORbj 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 your contribution! I rebased it and finalised it. Ready for merge!

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 16, 2023
@hbjORbj hbjORbj merged commit 6a32e4a into mui:master Mar 16, 2023
zIndex: 1000,
});

export default function MenuListComposition(): JSX.Element {
Copy link
Member

Choose a reason for hiding this comment

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

Since it's for the docs, I think we should keep the type simpler, even if slower:

Suggested change
export default function MenuListComposition(): JSX.Element {
export default function MenuListComposition() {

It's what the reset of the docs is doing, fixed in #38903.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants