feat: add impending deletion indicators to the workspace page #7588
Conversation
| }} | ||
| > | ||
| <Workspace {...args} /> | ||
| </ProxyContext.Provider> | ||
| <ProxyContext.Provider |
There was a problem hiding this comment.
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.
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.
nit: Since we are using the copy "Impending deletion" should we not call this ImpendingDeletionBadge?
There was a problem hiding this comment.
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.
In MUI v5 you can style things like ChakraUI:
<Box
component="span"
fontWeight={600}
color={theme => theme.palette.warning.light}
>
Impending Deletion
</Box>or
<Box
component="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.
Do we really need this? Why not access those components directly?
There was a problem hiding this comment.
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 with 1 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.
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.
…orkspace-page/kira-pilot
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.