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

[List] Remove background inheritance of ListSubheader #25532

Conversation

tanmoyopenroot
Copy link
Contributor

@tanmoyopenroot tanmoyopenroot commented Mar 28, 2021

  • Add check for breakpoint use sticky background from paper
  • Filter out onClick

Related to #18200

@mui-pr-bot
Copy link

mui-pr-bot commented Mar 28, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 65ee439

@oliviertassinari oliviertassinari changed the title [ListSubheader] Add select category support [List] Fix ListSubheader background color Mar 28, 2021
@oliviertassinari oliviertassinari added the component: list This is the name of the generic UI component, not the React module! label Mar 28, 2021
@oliviertassinari oliviertassinari changed the title [List] Fix ListSubheader background color [List] Remove background inheritance of ListSubheader Mar 28, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I have reduced the scope of the changes. Here, the objective is now to remove the background inheritance color approach to the dedicated background color. This is what we do with the Autocomplete component. It simplifies one of our demos and fixes an issue with the Select. IMHO, these are signals that the new approach performs better.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 28, 2021

Regarding the onclick event of #18200. The proposed fix looked completely wrong, leaking outside of the scope (this is a select issue, not a list subheader issue). I have reverted it.

@oliviertassinari oliviertassinari merged commit 4c0db62 into mui:next Mar 28, 2021
@tanmoyopenroot
Copy link
Contributor Author

tanmoyopenroot commented Mar 28, 2021

Regarding the onclick event of #18200. The proposed fix looked completely wrong, leaking outside of the scope (this is a select issue, not a list subheader issue). I have reverted it.

Ok, How about this way?
In SelectInput component (line), If value prop is undefined the we won't pass the onClick prop

@oliviertassinari
Copy link
Member

@tanmoyopenroot This path seems to have potential.

I would like to raise that #18200 is related to #14943. It's in the same order of how to handle composability well.

There is something else that caught my attention. The keyboard logic is already able to differentiate valid option from wrong ones. We should be able to leverage it. This rely on tabIndex={-1} coming from the MenuItem.
So, the alternative is, in the click event handler, skip it of the event.currentTarget doesn't have a tab index attribute.

@tanmoyopenroot
Copy link
Contributor Author

@tanmoyopenroot This path seems to have potential.

I would like to raise that #18200 is related to #14943. It's in the same order of how to handle composability well.

There is something else that caught my attention. The keyboard logic is already able to differentiate valid option from wrong ones. We should be able to leverage it. This rely on tabIndex={-1} coming from the MenuItem.
So, the alternative is, in the click event handler, skip it of the event.currentTarget doesn't have a tab index attribute.

Will try to fix this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: list This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants