- Notifications
You must be signed in to change notification settings - Fork913
feat: add impending deletion indicators to the workspace page#7588
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| }} | ||
| > | ||
| <Workspace {...args} /> | ||
| </ProxyContext.Provider> | ||
| <ProxyContext.Provider |
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.
Not sure if I'm correct, but maybe, this ProxyContext is already inside of the DashboardProvider so we don't need that here too?
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.
I can check!
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.
Sadly both are needed
| import { Pill } from "components/Pill/Pill" | ||
| import ErrorIcon from "@mui/icons-material/ErrorOutline" | ||
| export const DeletionBadge = ({ |
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.
nit: Since we are using the copy "Impending deletion" should we not call thisImpendingDeletionBadge?
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.
Sure! I can adjust the names
| const StyledSpan = styled.span<{ theme?: MaterialUITheme }>` | ||
| color: ${(props) => props.theme.palette.warning.light}; | ||
| font-weight: 600; |
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.
In MUI v5 you can style things like ChakraUI:
<Boxcomponent="span"fontWeight={600}color={theme=>theme.palette.warning.light}> Impending Deletion</Box>
or
<Boxcomponent="span"sx={{fontWeight:600,color:theme.palette.warning.light,}}> Impending Deletion</Box>
Just to let you know in case you find it better
| export * from "./DeletionStat" | ||
| export * from "./DeletionBadge" | ||
| export * from "./DeletionText" | ||
| export * from "./DeletionBanner" |
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.
Do we really need this? Why not access those components directly?
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.
Good q!
- Just a shorter directory to type out when importing, e.g.
components/WorkspaceDeletioninstead ofcomponents/WorkspaceDeletion/XYZ- I think it looks a little cleaner. This becomes a more important pattern if you have additional nesting. - I can also import multiple related components with1 line which keeps our file length down
- Often times, the barrel file provides a clue as to how the components are meant to be used. For example, in this case, I am exporting everything, which means all components are meant to be consumed. Great. Let's say I was building a table that had a specific widget component, broken apart in a separate file, but used in the table and colocated in the same directory. In the barrel I would export the table component, but not the widget. A user unfamiliar with the code base could glance at the barrel file and see exactly what was meant to be consumed externally, i.e. the table and not the widget.
| import { displayImpendingDeletion } from "./utils" | ||
| describe("util > workspace deletion", () => { | ||
| describe("displayImpendingDeletion", () => { |
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.
I think we could let only this describe here. The previous one didn't "make sense" to me since we are only testing this function.
Uh oh!
There was an error while loading.Please reload this page.
resolves#7350
I also used this PR as an opportunity to reorganize deletion components such that they are all colocated and contain their own display logic.