-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
[TreeView] Migrate TreeItem to emotion #25835
Conversation
@material-ui/lab: parsed: +0.25% , gzip: +0.16% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit trickier. The only problem I see is with the styles for the iconContainer
and the label
, but it is solvable. Let me know if you need some more help.
@oliviertassinari one question about theme styleOverrides for pseudo class. Is this the correct way to do in theme? createMuiTheme({
components: {
MuiTreeItem: {
styleOverrides: {
root: {
'& .Mui-focused': {
...styles
},
'& .Mui-expanded': {
...styles
}
}
}
}
}
}) |
@siriwatknp Yes |
@siriwatknp just watch out for the spacings, I believe it should be: createMuiTheme({
components: {
MuiTreeItem: {
styleOverrides: {
root: {
- '& .Mui-focused': {
+ '&.Mui-focused': {
...styles
},
- '& .Mui-expanded': {
+ '&.Mui-expanded': {
...styles
}
}
}
}
}
}) |
@@ -34,6 +34,10 @@ const useTreeItemStyles = makeStyles((theme) => ({ | |||
backgroundColor: `var(--tree-view-bg-color, ${theme.palette.action.selected})`, | |||
color: 'var(--tree-view-color)', | |||
}, | |||
'& $label': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use the global class here? For example:
'& $label': { | |
'& .MuiTreeItem-label': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even better:
'& $label': { | |
'& .${treeItemClasses.label}': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnajdova similar to @oliviertassinari suggestion, but there are many places still using $
in this file so I think it might be better to update the whole demo in another PR. is that okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, got it, sorry missed previous question I guess :)
Got your point, do I need to change? because right now the pseudo styles is at const StyledTreeItemContent = experimentalStyled(...)({
[`&.${treeItemClasses.disabled}`]: {
...
},
[`&.${treeItemClasses.focused}`]: {
...
},
[`&.${treeItemClasses.selected}`]: {
...
},
}) |
I would expect in that case maybe either : createMuiTheme({
components: {
MuiTreeItem: {
styleOverrides: {
content: { // target content
'&.Mui-focused': {
...styles
},
'&.Mui-expanded': {
...styles
}
}
}
}
}
}) or this createMuiTheme({
components: {
MuiTreeItem: {
styleOverrides: {
root: {
'& .MuiTreeItem-content.Mui-focused': {
...styles
},
'& .MuiTreeItem-content.Mui-expanded': {
...styles
}
}
}
}
}
}) |
Yep, I am good with this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
One chunk of #24405