fix(rbac): fixed ui issues #2061
Conversation
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Changed Packages
|
There was a problem hiding this comment.
Hi @ciiay, thanks, for the UI improvements! 👍
It would be nice if you can take a look into the comments. I think in some spots we can simplify the code and make it more theme-aware by using the special features of sx, like sx={{ p: 2 }}
See https://mui.com/system/getting-started/the-sx-prop/
On the other side we should be careful when enforcing any color. I think we shouldn't do it at all because we want that this plugin works well with the default Backstage theme and other really-customized themes.
| style={{ padding: '0.5rem', borderRadius: '50%' }} | ||
| sx={{ '&:hover': { borderRadius: '50%' } }} |
There was a problem hiding this comment.
Do we need both?
Fyi, with sx you can also define a padding, and there are also theme-aware properties like p that uses the theme gaps:
| style={{ padding: '0.5rem', borderRadius: '50%' }} | |
| sx={{ '&:hover': { borderRadius: '50%' } }} | |
| sx={{ p: 2, borderRadius: '50%', '&:hover': { borderRadius: '50%' } }} |
The 2 means 2x gap-size here, not 2px.
There was a problem hiding this comment.
Hi @christoph-jerolimov , if only set the hover to have borderRadius, when you hover it the ripple effect will be a square first then become a circle.
There was a problem hiding this comment.
Also if I specify in sx, it will be overwritten by Mui global css styles. So I had to use style to keep some styles in higher priority.
| @@ -66,6 +66,7 @@ function DownloadCSVLink() { | |||
| sx={{ | |||
| color: theme => theme.palette.link, | |||
| textDecoration: 'underline', | |||
| marginTop: '1rem', | |||
There was a problem hiding this comment.
| marginTop: '1rem', | |
| mt: 2, |
(I didn't checked if 2 x theme-gap-sizes matches what you tried here, but 2 is a common gap I guess.)
| style={{ padding: '0.5rem', color: 'inherit', borderRadius: '50%' }} | ||
| sx={{ '&:hover': { borderRadius: '50%' } }} |
There was a problem hiding this comment.
| style={{ padding: '0.5rem', color: 'inherit', borderRadius: '50%' }} | |
| sx={{ '&:hover': { borderRadius: '50%' } }} | |
| sx={{ p: 1, color: 'inherit', borderRadius: '50%', '&:hover': { borderRadius: '50%' } }} |
(I didn't checked it visually, but also here a theme-based-gap would be better than 0.5rem, so that the theme can configure the gaps.
| const dialogBackgroundColor = (theme: { palette: { mode: string } }) => | ||
| theme.palette.mode === 'dark' ? '#1b1d21' : '#fff'; | ||
|
|
There was a problem hiding this comment.
That's to specific. Is there no property that you can use from theme.palette.... ?
This might be an issue with our theme. We should not enforce (our) colors here.
| style={{ marginTop: '24px' }} | ||
| sx={{ | ||
| marginTop: '24px', | ||
| backgroundColor: `${dialogBackgroundColor} !important`, |
There was a problem hiding this comment.
Nope. I think its a theme issue on our side.
If you really* want pick up the background color of the DialogContent you can make it transparent. But I think it's to risky to have bad contrast between the text color and the background color.
| @@ -66,7 +66,7 @@ export const RolesListToolbar = ({ | |||
| sx={{ | |||
| display: 'flex', | |||
| justifyContent: 'end', | |||
| marginBottom: '24px', | |||
| marginBottom: '24px !important', | |||
There was a problem hiding this comment.
Is this really needed? Can you try
| marginBottom: '24px !important', | |
| mb: 3, |
However, it should be the same CSS specificity. Hmm. I must take a look into the browser dev tools myself.
There was a problem hiding this comment.
This won't work as inline sx styles will be overridden by Mui global styles.
Signed-off-by: Yi Cai <[email protected]>
Hey, I just made a Pull Request!
For fix of UI issues of RBAC plugin.
✔️ Checklist
Signed-off-byline in the message. (more info)