[TreeView] Simplify customization #21514
Merged
Conversation
This file contains 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
joshwooding
added
the
component: tree view
joshwooding
force-pushed
the
new-tree-tidy-up
branch
from
847d3d6
to
a4647f2
Compare
Details of bundle changes.Comparing: 48ad4e9...54019eb Details of page changes
|
joshwooding
commented
Jun 21, 2020
| label: { | ||
| fontWeight: 'inherit', | ||
| color: 'inherit', | ||
| }, | ||
| labelRoot: { | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| padding: theme.spacing(0.5, 0), | ||
| padding: theme.spacing(0.5, 0, 0.5, 0.5), |
There was a problem hiding this comment.
Included from the other PR.
oliviertassinari
changed the title
oliviertassinari
approved these changes
Jun 21, 2020
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
|
@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. |
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? |
|
Yeah, I agree. Everything looks good. |
eps1lon
requested changes
Jun 22, 2020
|
@eps1lon TreeItemClassKey has been updated? The only new class is |
eps1lon
approved these changes
Jun 23, 2020
eps1lon
reviewed
Jun 23, 2020
| @@ -189,6 +197,7 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { | |||
|
|
|||
| const handleMouseDown = (event) => { | |||
| if (event.shiftKey || event.ctrlKey || event.metaKey) { | |||
| // Prevent text selection | |||
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?
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.
|
I thought |
2 tasks
jonenst
added a commit
to gridsuite/commons-ui
that referenced
this pull request
May 23, 2023
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
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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