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

[MenuRoot] Remove wrong default value from docs #549

Merged
merged 7 commits into from
Aug 27, 2024

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Aug 21, 2024

I was looking at source code of Menu and docs, docs says default value of openOnHover prop is nested but nested value is not defined in code and even if openOnHover prop is defined as nested following code (see line 27) would be illogical as openOnHoverProp will always be true. hence i removed default prop as nested from docs.

openOnHover: openOnHoverProp,
} = props;
const parentContext = useMenuRootContext(true);
const nested = parentContext != null;
const openOnHover = openOnHoverProp ?? nested;
const menuRoot = useMenuRoot({
animated,

Before:

image

After

image

Preview:

https://deploy-preview-549--base-ui.netlify.app/base-ui/react-menu/components-api/#menu-root-props

@sai6855 sai6855 marked this pull request as draft August 21, 2024 10:00
@mui-bot
Copy link

mui-bot commented Aug 21, 2024

Netlify deploy preview

https://deploy-preview-549--base-ui.netlify.app/

Generated by 🚫 dangerJS against a15fc02

@sai6855 sai6855 marked this pull request as ready for review August 21, 2024 10:25
@zannager zannager added component: menu This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation labels Aug 21, 2024
@@ -139,8 +139,6 @@ namespace MenuRoot {
delay?: number;
/**
* Whether the menu popup opens when the trigger is hovered after the provided `delay`.
*
* @default nested
Copy link
Member

Choose a reason for hiding this comment

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

I used to have such a prop in one of the previous implementation attempts and forgot to remove the @default annotation. We can add in plain text that by default openOnHover is set on nested menus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -139,6 +139,7 @@ namespace MenuRoot {
delay?: number;
/**
* Whether the menu popup opens when the trigger is hovered after the provided `delay`.
* By default, openOnHover is set to `true` for nested menus.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* By default, openOnHover is set to `true` for nested menus.
* By default, `openOnHover` is set to `true` for nested menus.

@michaldudak
Copy link
Member

Thanks!

@michaldudak michaldudak merged commit 3b042f5 into mui:master Aug 27, 2024
18 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants