Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(rbac): upgrade to mui5 #1985

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+644 −513
Open

feat(rbac): upgrade to mui5 #1985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions workspaces/rbac/.changeset/grumpy-foxes-smash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@backstage-community/plugin-rbac': minor
---

Upgraded RBAC plugin from mui4 to mui5
4 changes: 1 addition & 3 deletions workspaces/rbac/plugins/rbac/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,9 @@
"@backstage/plugin-permission-react": "^0.4.27",
"@backstage/theme": "^0.6.0",
"@janus-idp/shared-react": "^2.13.1",
"@material-ui/core": "^4.9.13",
"@material-ui/icons": "^4.11.3",
"@material-ui/lab": "^4.0.0-alpha.45",
Comment on lines -49 to -51
Copy link
Member

Choose a reason for hiding this comment

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

🥳

"@mui/icons-material": "5.16.4",
"@mui/material": "^5.14.18",
"@mui/styles": "^6.1.7",
"@rjsf/core": "^5.21.2",
"@rjsf/mui": "^5.21.2",
"@rjsf/utils": "^5.21.2",
Expand Down
7 changes: 4 additions & 3 deletions workspaces/rbac/plugins/rbac/src/components/ConditionalAccess/AddNestedConditionButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@
*/
import React from 'react';

import { Box, Typography } from '@material-ui/core';
import Tooltip from '@material-ui/core/Tooltip';
import HelpOutlineIcon from '@material-ui/icons/HelpOutline';
import HelpOutlineIcon from '@mui/icons-material/HelpOutline';
import Box from '@mui/material/Box';
import Tooltip from '@mui/material/Tooltip';
import Typography from '@mui/material/Typography';

export const AddNestedConditionButton = () => {
const tooltipTitle = () => (
Expand Down
11 changes: 7 additions & 4 deletions workspaces/rbac/plugins/rbac/src/components/ConditionalAccess/ComplexConditionRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ import React from 'react';

import { PermissionCondition } from '@backstage/plugin-permission-common';

import { IconButton } from '@material-ui/core';
import RemoveIcon from '@mui/icons-material/Remove';
import IconButton from '@mui/material/IconButton';

import {
getNestedRuleErrors,
Expand Down Expand Up @@ -51,7 +51,6 @@ type ComplexConditionRowProps = {
setRemoveAllClicked: React.Dispatch<React.SetStateAction<boolean>>;
conditionRulesData?: RulesData;
notConditionType?: NotConditionType;
classes: any;
currentCondition: Condition;
ruleIndex: number;
activeCriteria?: 'allOf' | 'anyOf';
Expand All @@ -70,7 +69,6 @@ export const ComplexConditionRow = ({
setRemoveAllClicked,
conditionRulesData,
notConditionType,
classes,
currentCondition,
ruleIndex,
activeCriteria,
Expand Down Expand Up @@ -252,7 +250,12 @@ export const ComplexConditionRow = ({
/>
<IconButton
title="Remove"
className={classes.removeRuleButton}
sx={{
color: theme => theme.palette.grey[500],
flexGrow: 0,
alignSelf: 'baseline',
marginTop: theme => theme.spacing(3.3),
}}
disabled={isNestedCondition ? nestedDisabled : disabled}
onClick={
isNestedCondition &&
Expand Down
14 changes: 10 additions & 4 deletions workspaces/rbac/plugins/rbac/src/components/ConditionalAccess/ComplexConditionRowButtons.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ type ComplexConditionRowButtonsProps = {
conditionRow: ConditionsData;
onRuleChange: (newCondition: ConditionsData) => void;
criteria: string;
classes: any;
selPluginResourceType: string;
updateErrors: (_index: number) => void;
isNestedConditionRule: (condition: Condition) => boolean;
Expand All @@ -39,7 +38,6 @@ export const ComplexConditionRowButtons = ({
conditionRow,
onRuleChange,
criteria,
classes,
selPluginResourceType,
updateErrors,
isNestedConditionRule,
Expand Down Expand Up @@ -74,15 +72,23 @@ export const ComplexConditionRowButtons = ({
(criteria === criterias.allOf || criteria === criterias.anyOf) && (
<Box mt={1} mb={1}>
<Button
className={classes.addRuleButton}
sx={{
display: 'flex !important',
color: theme => theme.palette.primary.light,
textTransform: 'none',
}}
size="small"
onClick={handleAddRule}
>
<AddIcon fontSize="small" />
Add rule
</Button>
Comment on lines 74 to 85
Copy link
Member

Choose a reason for hiding this comment

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

I know, this was mostly the same code as before. But instead of custom styles the Button supports this props as well. Can you check if this looks the same:

<Button color="primary" size="small" startIcon={<Add Icon fontSize="small" />}>
  Add rule
</Button>

If primary color light and primary color isn't the same here, we can maybe anyway use startIcon?

<Button
className={classes.addNestedConditionButton}
sx={{
display: 'flex !important',
Copy link
Member

Choose a reason for hiding this comment

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

As above, I think we can use the Button prop startIcon instead.

color: theme => theme.palette.primary.light,
textTransform: 'none',
}}
size="small"
onClick={() => handleAddNestedCondition(criteria)}
>
Expand Down
80 changes: 37 additions & 43 deletions workspaces/rbac/plugins/rbac/src/components/ConditionalAccess/ConditionalAccessSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,50 +15,15 @@
*/
import React from 'react';

import { makeStyles } from '@material-ui/core';
import Drawer from '@material-ui/core/Drawer';
import CloseIcon from '@mui/icons-material/Close';
import Box from '@mui/material/Box';
import Drawer from '@mui/material/Drawer';
import IconButton from '@mui/material/IconButton';
import Typography from '@mui/material/Typography';

import { ConditionsForm } from './ConditionsForm';
import { ConditionsData, RulesData } from './types';

const useDrawerStyles = makeStyles(() => ({
paper: {
['@media (max-width: 960px)']: {
width: '-webkit-fill-available',
},
width: '50vw',
height: '100vh',
gap: '3%',
display: '-webkit-inline-box',
},
}));

const useDrawerContentStyles = makeStyles(theme => ({
sidebar: {
display: 'flex',
flexFlow: 'column',
justifyContent: 'space-between',
backgroundColor: `${theme.palette.background.default} !important`,
},
header: {
display: 'flex',
flexDirection: 'row',
justifyContent: 'space-between',
alignItems: 'baseline',
padding: theme.spacing(2.5),
fontFamily: theme.typography.fontFamily,
},
headerSubtitle: {
fontWeight: 400,
fontFamily: theme.typography.fontFamily,
paddingTop: theme.spacing(1),
},
}));

type ConditionalAccessSidebarProps = {
open: boolean;
onClose: () => void;
Expand All @@ -76,27 +41,56 @@ export const ConditionalAccessSidebar = ({
conditionRulesData,
conditionsFormVal,
}: ConditionalAccessSidebarProps) => {
const classes = useDrawerStyles();
const contentClasses = useDrawerContentStyles();
return (
<Drawer
anchor="right"
open={open}
data-testid="rules-sidebar"
classes={{
paper: classes.paper,
sx={{
['@media (max-width: 960px)']: {
width: '-webkit-fill-available',
},
width: '50vw',
height: '100vh',
gap: '3%',
display: '-webkit-inline-box',
Copy link
Member

Choose a reason for hiding this comment

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

I see that you just moved the code. But anyway:

  • Is this really necessary?
  • Does this work well on Firefox?

['& div.MuiDrawer-paper']: {
width: '0px',
},
Comment on lines +57 to +59
Copy link
Member

Choose a reason for hiding this comment

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

We try to hide what element here?

}}
>
<Box className={contentClasses.sidebar}>
<Box className={contentClasses.header}>
<Box
sx={{
display: 'flex',
flexFlow: 'column',
justifyContent: 'space-between',
height: '100%',
backgroundColor: theme =>
`${theme.palette.background.default} !important`,
Comment on lines +68 to +69
Copy link
Member

Choose a reason for hiding this comment

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

sx also support strings for picking up the theme. Idk what is "better". But please remove all !important, that shouldn't be necessary, or?

}}
>
<Box
sx={{
display: 'flex',
flexDirection: 'row',
justifyContent: 'space-between',
alignItems: 'baseline',
padding: theme => theme.spacing(2.5),
fontFamily: theme => theme.typography.fontFamily,
}}
>
<Typography variant="h5">
<Typography component="span" sx={{ fontWeight: 500 }}>
Configure access for the
</Typography>{' '}
{selPluginResourceType}
<Typography
variant="body2"
className={contentClasses.headerSubtitle}
sx={{
fontWeight: 400,
fontFamily: theme => theme.typography.fontFamily,
paddingTop: theme => theme.spacing(1),
}}
align="left"
>
By default, the selected resource type will be visible to the
Expand Down
5 changes: 0 additions & 5 deletions workspaces/rbac/plugins/rbac/src/components/ConditionalAccess/ConditionsForm.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,6 @@ import { fireEvent, render, screen } from '@testing-library/react';
import { mockTransformedConditionRules } from '../../__fixtures__/mockTransformedConditionRules';
import { ConditionsForm } from './ConditionsForm';

jest.mock('@material-ui/core', () => ({
...jest.requireActual('@material-ui/core'),
makeStyles: jest.fn().mockReturnValue(() => ({})),
}));

describe('ConditionsForm', () => {
const selPluginResourceType = 'catalog-entity';
const onSaveMock = jest.fn();
Expand Down
53 changes: 25 additions & 28 deletions workspaces/rbac/plugins/rbac/src/components/ConditionalAccess/ConditionsForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ import React from 'react';

import { PermissionCondition } from '@backstage/plugin-permission-common';

import { Box, Button, makeStyles } from '@material-ui/core';
import { Alert, AlertTitle } from '@material-ui/lab';
import WarningIcon from '@mui/icons-material/Warning';
import Alert from '@mui/material/Alert';
import AlertTitle from '@mui/material/AlertTitle';
import Box from '@mui/material/Box';
import Button from '@mui/material/Button';

import {
hasAllOfOrAnyOfErrors,
Expand All @@ -38,29 +40,6 @@ import {
RulesData,
} from './types';

const useStyles = makeStyles(theme => ({
form: {
padding: theme.spacing(2.5),
paddingTop: 0,
flexGrow: 1,
overflow: 'auto',
},
addConditionButton: {
color: theme.palette.primary.light,
},
footer: {
display: 'flex',
flexDirection: 'row',
gap: '15px',
alignItems: 'baseline',
borderTop: `2px solid ${theme.palette.border}`,
padding: theme.spacing(2.5),
'& button': {
textTransform: 'none',
},
},
}));

type ConditionFormProps = {
conditionRulesData?: RulesData;
conditionsFormVal?: ConditionsData;
Expand All @@ -76,7 +55,6 @@ export const ConditionsForm = ({
onClose,
onSave,
}: ConditionFormProps) => {
const classes = useStyles();
const [conditions, setConditions] = React.useState<ConditionsData>(
conditionsFormVal ?? {
condition: {
Expand Down Expand Up @@ -217,7 +195,14 @@ export const ConditionsForm = ({

return (
<>
<Box className={classes.form}>
<Box
sx={{
padding: theme => theme.spacing(2.5),
paddingTop: 0,
flexGrow: 1,
overflow: 'auto',
}}
>
<ConditionsFormRow
conditionRulesData={conditionRulesData}
conditionRow={conditions}
Expand All @@ -243,7 +228,19 @@ export const ConditionsForm = ({
</Alert>
)}
</Box>
<Box className={classes.footer}>
<Box
sx={{
display: 'flex',
flexDirection: 'row',
gap: '15px',
alignItems: 'baseline',
borderTop: theme => `2px solid ${theme.palette.border}`,
padding: theme => theme.spacing(2.5),
'& button': {
textTransform: 'none',
},
}}
>
<Button
variant="contained"
color="primary"
Expand Down
Loading
Loading