[TreeView] Simplify customization#21514
Merged
oliviertassinari merged 4 commits intomui:nextfrom Jun 24, 2020
Merged
Conversation
847d3d6 to
a4647f2
Compare
Details of bundle changes.Comparing: 48ad4e9...54019eb Details of page changes
|
joshwooding
commented
Jun 21, 2020
| display: 'flex', | ||
| alignItems: 'center', | ||
| padding: theme.spacing(0.5, 0), | ||
| padding: theme.spacing(0.5, 0, 0.5, 0.5), |
Collaborator
Author
There was a problem hiding this comment.
Included from the other PR.
oliviertassinari
approved these changes
Jun 21, 2020
Member
oliviertassinari
left a comment
There was a problem hiding this comment.
@joshwooding I have added a new commit with the following changes:
- differentiate "selected + hover" to the "selected + focused" style
- Increase the specificity of the Gmail demo to override all the cases
Collaborator
Author
|
@oliviertassinari Looks good to me. I think we made selected + hover the same as selected + focus to make it simpler but I’m happy to have them separated. I was tempted to do the same for the Gmail tree but I think it made it harder to see where focus matched selected. |
Member
Yeah, I wonder about this tradeoff. It seems that the purpose of changing the style is double: 1. to identify if the element can be interacted with, 2. to identify where the element is. I think that with With the previous tradeoff, it felt that for keyboard users, the visual diff wasn't strong enough to identify where the focused element was. I have tried changing the opacity to use the focus one (instead of the hover one), which adds more contrast, however, it felt too strong for the hover case. What do you think? |
Collaborator
Author
|
Yeah, I agree. Everything looks good. |
eps1lon
requested changes
Jun 22, 2020
Member
eps1lon
left a comment
There was a problem hiding this comment.
Aren't we missing updates to the *ClassKey in the corresponding typescript declarations?
Collaborator
Author
|
@eps1lon TreeItemClassKey has been updated? The only new class is |
eps1lon
approved these changes
Jun 23, 2020
eps1lon
reviewed
Jun 23, 2020
|
|
||
| const handleMouseDown = (event) => { | ||
| if (event.shiftKey || event.ctrlKey || event.metaKey) { | ||
| // Prevent text selection |
Member
There was a problem hiding this comment.
Doesn't this also prevent focus in certain browsers or do those only focus if no meta keys are pressed?
Collaborator
Author
There was a problem hiding this comment.
You're right but because we don't depend on the focusing of a tree item for anything since we call focus in the click handler it shouldn't matter. But it might trip up someone depending on this behaviour. I have plans to change this part of the tree anyway.
Member
|
I thought |
2 tasks
jonenst
added a commit
to gridsuite/commons-ui
that referenced
this pull request
May 23, 2023
After a quick look, we didn' customize TreeViewFinder which also uses TreeView/TreeItem so it shouldn't need the same fixing as ReportViewer. in mui/material-ui#21514 pseudo classes were moved from root to content, so we no longer need nested selectors and rules need to go on content directly use Mui-focused instead of :focus also restore muiv4 treeitem label color rule to get a different color right after a click on an item. It was also removed in treeitem in mui/material-ui#21514 Note: in v5 primary.main was changed from indigo to blue in mui/material-ui#26555) but let's go with the new primary colors Note2: fade renamed to alpha
EstherDarkish
pushed a commit
to gridsuite/commons-ui
that referenced
this pull request
Jun 7, 2023
After a quick look, we didn' customize TreeViewFinder which also uses TreeView/TreeItem so it shouldn't need the same fixing as ReportViewer. in mui/material-ui#21514 pseudo classes were moved from root to content, so we no longer need nested selectors and rules need to go on content directly use Mui-focused instead of :focus also restore muiv4 treeitem label color rule to get a different color right after a click on an item. It was also removed in treeitem in mui/material-ui#21514 Note: in v5 primary.main was changed from indigo to blue in mui/material-ui#26555) but let's go with the new primary colors Note2: fade renamed to alpha Co-authored-by: Sylvain Bouzols <sylvain.bouzols@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alternative to #20159
Closes #20125
Closes #20311