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

[Menu][base] Add the resetHighlight action #37392

Merged
merged 3 commits into from
Jun 16, 2023

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented May 24, 2023

This change enables solving the problem described in #36857 (review).

The issue was that after resetting the filter, the highlight remained at the first filtered item.
There are cases where this behavior is desired - for example, when lazy loading more options, we don't want the highlight to jump to the beginning. That's why I decided to leave the decision to reset the highlight to the developer. See https://codesandbox.io/s/cranky-jennings-954ghk?file=/demo.tsx for an example of how to solve it.

Initially, we discussed exposing just the high-level functions in the actions ref of the Menu (resetHighlight). However, this may not be enough as we need more actions to be dispatched, such as keyDown and buttonClick (they are used to implement our demos, so developers will likely need to use them). When we implement the MenuButton, we probably won't have to expose so many actions.
I ended up exposing both resetHighlight (for ease of use) and dispatch (for completeness). Let me know if you think it's redundant. I'm not sure what's the best option here.

Closes #36800

@michaldudak michaldudak added component: menu This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base enhancement This is not a bug, nor a new feature labels May 24, 2023
@mui-bot
Copy link

mui-bot commented May 24, 2023

Netlify deploy preview

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

@material-ui/unstyled: parsed: +0.27% , gzip: +0.24%
@mui/joy: parsed: +0.10% , gzip: +0.09%

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 233ea5c

@michaldudak michaldudak added this to the MUI Base stable release milestone May 24, 2023
@michaldudak michaldudak marked this pull request as ready for review May 24, 2023 21:12
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 LGTM. Regarding the demos, I think they need to be simplified. It's quite intimidating with a lot of useRef at the moment.

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

Regarding the demos, I think they need to be simplified. It's quite intimidating with a lot of useRef at the moment.

Yeah, I agree. The major reason for this is the lack of the MenuButton component. Once we have this, demos will become much simpler.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 12, 2023
@michaldudak michaldudak merged commit 7a2ae61 into mui:master Jun 16, 2023
@michaldudak michaldudak deleted the base-menu-actions branch June 16, 2023 14:07
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! enhancement This is not a bug, nor a new feature package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[joy][base][Menu] MenuItem tabindex isn't logical in a changing list
3 participants