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

[base] Maintain nodes document order in compound components #36857

Merged

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Apr 12, 2023

Makes sure that the registered compound component's children are in document order, even if inserted in the middle of the existing collection.

Part of the fix for #36800

Before: https://codesandbox.io/s/modest-gates-zwyhbd?file=/demo.tsx
After: https://codesandbox.io/s/naughty-banzai-3d6e6z?file=/demo.tsx

@michaldudak michaldudak added bug 🐛 Something doesn't work package: base-ui Specific to @mui/base labels Apr 12, 2023
@michaldudak michaldudak requested a review from mnajdova April 12, 2023 10:01
@mui-bot
Copy link

mui-bot commented Apr 12, 2023

Netlify deploy preview

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

@material-ui/unstyled: parsed: +0.39% , gzip: +0.45%
@mui/material-next: parsed: +0.23% , gzip: +0.28%
@mui/joy: parsed: +0.11% , gzip: +0.17%

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against ac5a469

Copy link
Contributor

@sai6855 sai6855 left a comment

Choose a reason for hiding this comment

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

@michaldudak i could still reproduce the issue though.

  1. enter "d"
  2. clear "d"
  3. press "Tab"

here i think 1st option should be highlighted but right now 2nd option is highlighted

https://www.loom.com/share/912b0018a0b74204affba593f87b1b00

@michaldudak
Copy link
Member Author

michaldudak commented Apr 12, 2023

Ahh, OK, it's a different issue now (that's actually a feature in other cases). What happens is that the Menu tries to keep the highlighted item when options change. This is handy when you load more options lazily and don't want the scroll to reset (we recently had this issue in Material UI's Autocomplete).

Let me think for a bit about how to approach this to handle both cases. I suppose the lazy loading case won't be common for menus, so we can always reset the highlight. On the other hand, I didn't expect to see a Menu with filtering functionality...

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

@siriwatknp This should fix the problem with Base UI's marketing page's Select highlighting the middle item.
I'll fix the remainder of the #36800 issue in a separate PR, as it's a different problem.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@michaldudak michaldudak merged commit 5f642d5 into mui:master May 24, 2023
@michaldudak michaldudak deleted the iss/36800-compound-components-order branch May 24, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants