Skip to content

[docs] Migrate TreeView demos#26146

Merged
mnajdova merged 4 commits into
mui:nextfrom
mnajdova:docs/components-pages
May 6, 2021
Merged

[docs] Migrate TreeView demos#26146
mnajdova merged 4 commits into
mui:nextfrom
mnajdova:docs/components-pages

Conversation

@mnajdova

@mnajdova mnajdova commented May 5, 2021

Copy link
Copy Markdown
Member

@mui-pr-bot

mui-pr-bot commented May 5, 2021

Copy link
Copy Markdown

No bundle size changes

Generated by 🚫 dangerJS against ec528e8

@oliviertassinari oliviertassinari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good

Comment thread docs/src/pages/components/tree-view/FileSystemNavigator.js
content: {
const StyledTreeItemRoot = experimentalStyled(TreeItem)(({ theme }) => ({
color: theme.palette.text.secondary,
[`& .${treeItemClasses.content}`]: {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting, it's seems that we didn't use them in the past. What should we do going forward?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My thinking was, if there are more styles, especially with multiple keys to style a component, styled() looked like the better replacement (I just replaced the keys in the classes with the classes selectors). For trivial, one line changes I used the sx prop.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the concept of being able to trust already defined class names instead of typing and making a mistake on it.

It looks interesting to use the same technique that is already used in the creation of the component.

@oliviertassinari oliviertassinari May 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My point was exactly on what @vicasas, the use of the classes object. No real preferences. I like how a plain string is YOLO and often good enough, but I also understand how having a JS object with IntelliSense that describe what each key is for can also help.

Comment thread docs/src/pages/components/tree-view/GmailTreeView.tsx Outdated
Comment thread docs/src/pages/components/tree-view/GmailTreeView.tsx Outdated
@oliviertassinari oliviertassinari added scope: tree view Changes related to the tree view. This includes TreeView, TreeItem. docs Improvements or additions to the documentation. labels May 5, 2021
@oliviertassinari

Copy link
Copy Markdown
Member

cc @vicasas that has worked on this type of efforts so far, in case he wants to review.

Comment thread docs/src/pages/components/tree-view/BarTreeView.js Outdated
'Mui-expanded': expanded,
'Mui-selected': selected,
'Mui-focused': focused,
'Mui-disabled': disabled,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think it is a bit of a different use-case, but I see your point. I would say that for the pseudo states, we should always rely on the pseudo classes, as we do with the core components, as sometimes it would be impossible to override some css rules if we don't bump the specificity when overriding.

For example, for the disabled state, the core components are defining the styles with:

'&.Mui-disabled': { ... },

If we don't bump the specificity in the overrides, we won't be able to override.

@mnajdova
mnajdova merged commit 925bbaf into mui:next May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to the documentation. scope: tree view Changes related to the tree view. This includes TreeView, TreeItem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants