Skip to content

[TreeView] Migrate TreeItem to emotion#25835

Merged
mnajdova merged 12 commits intomui:nextfrom
siriwatknp:tree-item-emotion
Apr 27, 2021
Merged

[TreeView] Migrate TreeItem to emotion#25835
mnajdova merged 12 commits intomui:nextfrom
siriwatknp:tree-item-emotion

Conversation

@siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Apr 19, 2021

One chunk of #24405

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 19, 2021

Details of bundle changes

@material-ui/lab: parsed: +0.25% , gzip: +0.16%

Generated by 🚫 dangerJS against a3234ed

@siriwatknp siriwatknp added the scope: tree view Changes related to the tree view. This includes TreeView, TreeItem. label Apr 19, 2021
@siriwatknp siriwatknp requested a review from mnajdova April 21, 2021 16:44
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.

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.

@siriwatknp siriwatknp requested a review from mnajdova April 23, 2021 16:34
@siriwatknp
Copy link
Member Author

siriwatknp commented Apr 25, 2021

@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
          }
        }
      }
    }
  }
})

@oliviertassinari
Copy link
Member

@siriwatknp Yes

@mnajdova
Copy link
Member

@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 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
          }
        }
      }
    }
  }
})

backgroundColor: `var(--tree-view-bg-color, ${theme.palette.action.selected})`,
color: 'var(--tree-view-color)',
},
'& $label': {
Copy link
Member

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:

Suggested change
'& $label': {
'& .MuiTreeItem-label': {

Copy link
Member

Choose a reason for hiding this comment

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

Or even better:

Suggested change
'& $label': {
'& .${treeItemClasses.label}': {

Copy link
Member Author

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?

Copy link
Member

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 :)

@siriwatknp
Copy link
Member Author

siriwatknp commented Apr 26, 2021

@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 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
          }
        }
      }
    }
  }
})

Got your point, do I need to change? because right now the pseudo styles is at Content, not Root.

const StyledTreeItemContent = experimentalStyled(...)({
  [`&.${treeItemClasses.disabled}`]: {
    ...
  },
  [`&.${treeItemClasses.focused}`]: {
    ...
  },
  [`&.${treeItemClasses.selected}`]: {
    ...
  },
})

@mnajdova
Copy link
Member

Got your point, do I need to change? because right now the pseudo styles is at Content, not Root.

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
          }
        }
      }
    }
  }
})

@siriwatknp
Copy link
Member Author

Got your point, do I need to change? because right now the pseudo styles is at Content, not Root.

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.

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.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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