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] Improve hover/select/focus UI display #21930

Merged
merged 2 commits into from
Aug 3, 2020

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Jul 25, 2020

Closes #5186
Related to #19343

@joshwooding joshwooding added design: material This is about Material Design, please involve a visual or UX designer in the process component: list This is the name of the generic UI component, not the React module! labels Jul 25, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Jul 25, 2020

@material-ui/core: parsed: +0.12% , gzip: +0.04%

Details of bundle changes

Generated by 🚫 dangerJS against 5de02d6

@oliviertassinari
Copy link
Member

@joshwooding What's missing from the pull request?

@joshwooding joshwooding marked this pull request as ready for review August 1, 2020 17:18
@joshwooding
Copy link
Member Author

@joshwooding What's missing from the pull request?

Nothing :) Had something I wanted to double-check but it was fine.

@oliviertassinari
Copy link
Member

The logic looks really close to what we have on Pagination and TreeView. I think that we can scale it to all the other components now :)

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.

It's much more clear now what the there states represents. Do we want to add a combination of focusedVisible + hover? It won't be a problem as we cannot have a conflict, but just something to consider maybe? That way the combination of selectors will always be predictable when the opacity is calculated (we are doing it for selected + other states, why not between hover and other states too).

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 2, 2020

Do we want to add a combination of focusedVisible + hover?

From what I recall during the initial implementation of the states styles (for the new MD specs), we made as many simplifications as possible in order to simplify overrides and the logic itself. We have ignored a bunch of possible states.

Later on, we realized that it was important to differentiate the opacity style of "focus visible" vs. "hover" because they had significant different use cases (with focus visible, you need more contrast than a hover. But at the same time, focus visible is too much contrast for a hover), so we added coverage for it.

Regarding the combinatory of the two states. Is the added complexity worth it?

@mnajdova
Copy link
Member

mnajdova commented Aug 2, 2020

Regarding the combinatory of the two states. Is the added complexity worth it?

Probably not. My point was, to me it was not clear why we would combine some states and not others, and where is this documented and how can we make sure it will be consistent across the components. On the other hand if I know that I always need to combine state, then we could be more confident that the consistency will be kept.

Combination of opacity does not have to be the answer for this, but it would be useful, at least for me if we have some form of documentation or guide on how do we combine states. Or if we don't combine all of them, than maybe it would be better to directly define in the theme the combinations that we support instead of doing calculation sometimes, but not always.

Anyway all this is probably just for my clarification and should not block this PR if the colors used now are by design.

@eps1lon eps1lon merged commit 08c4736 into mui:next Aug 3, 2020
kodai3 pushed a commit to kodai3/material-ui that referenced this pull request Aug 3, 2020
@eps1lon eps1lon added this to the v5 milestone Aug 4, 2020
@eps1lon eps1lon mentioned this pull request Aug 5, 2020
42 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: list This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Menu] Improve hover/select/focus UI display
6 participants