-
Notifications
You must be signed in to change notification settings - Fork 899
feat: add 'impending deletion' badges to workspaces page #7530
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
0ba53bd
1255b07
3618c5d
d6adbd9
e00ed2a
2dfbb50
3c72d11
fc8e51c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,12 +3,14 @@ import ErrorIcon from "@mui/icons-material/ErrorOutline" | |
import StopIcon from "@mui/icons-material/StopOutlined" | ||
import PlayIcon from "@mui/icons-material/PlayArrowOutlined" | ||
import QueuedIcon from "@mui/icons-material/HourglassEmpty" | ||
import { WorkspaceBuild } from "api/typesGenerated" | ||
import { Workspace, WorkspaceBuild } from "api/typesGenerated" | ||
import { Pill } from "components/Pill/Pill" | ||
import i18next from "i18next" | ||
import { FC, PropsWithChildren } from "react" | ||
import { makeStyles } from "@mui/styles" | ||
import { combineClasses } from "utils/combineClasses" | ||
import { displayImpendingDeletion } from "utils/workspace" | ||
import { useDashboard } from "components/Dashboard/DashboardProvider" | ||
|
||
const LoadingIcon: FC = () => { | ||
return <CircularProgress size={10} style={{ color: "#FFF" }} /> | ||
|
@@ -87,22 +89,49 @@ export const getStatus = (buildStatus: WorkspaceBuild["status"]) => { | |
} | ||
|
||
export type WorkspaceStatusBadgeProps = { | ||
build: WorkspaceBuild | ||
workspace: Workspace | ||
className?: string | ||
} | ||
|
||
const ImpendingDeletionBadge: FC< | ||
PropsWithChildren<Partial<WorkspaceStatusBadgeProps>> | ||
> = ({ className }) => { | ||
const { entitlements, experiments } = useDashboard() | ||
const allowAdvancedScheduling = | ||
entitlements.features["advanced_template_scheduling"].enabled | ||
// This check can be removed when https://github.com/coder/coder/milestone/19 | ||
// is merged up | ||
const allowWorkspaceActions = experiments.includes("workspace_actions") | ||
|
||
if (!allowAdvancedScheduling || !allowWorkspaceActions) { | ||
return null | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are already checking if we should or not display the badge on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @BrunoQuaresma those checks rely on a hook, which has to be called in a component. Would you rather I call the hook in the Alternatively, I thought about calling this hook in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thing for me is to have the "should display or not" check in one single place. Splitting this check into different places looks confusing to me. But if it is hard, I would add a comment so that in the future, if we want to change the display behavior, we know there are two places to look for. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed - as the ideal behavior is to have the check in one place, I think the best path forward may be to call the hook in the badge component and then ignore Storybook's complaints, rather than let our testing tool dictate how we code. |
||
return ( | ||
<Pill | ||
className={className} | ||
icon={<ErrorIcon />} | ||
text="Impending deletion" | ||
type="error" | ||
/> | ||
) | ||
} | ||
|
||
export const WorkspaceStatusBadge: FC< | ||
PropsWithChildren<WorkspaceStatusBadgeProps> | ||
> = ({ build, className }) => { | ||
const { text, icon, type } = getStatus(build.status) | ||
> = ({ workspace, className }) => { | ||
if (displayImpendingDeletion(workspace)) { | ||
return <ImpendingDeletionBadge className={className} /> | ||
} | ||
|
||
const { text, icon, type } = getStatus(workspace.latest_build.status) | ||
return <Pill className={className} icon={icon} text={text} type={type} /> | ||
} | ||
|
||
export const WorkspaceStatusText: FC< | ||
PropsWithChildren<WorkspaceStatusBadgeProps> | ||
> = ({ build, className }) => { | ||
> = ({ workspace, className }) => { | ||
const styles = useStyles() | ||
const { text, type } = getStatus(build.status) | ||
const { text, type } = getStatus(workspace.latest_build.status) | ||
return ( | ||
<span | ||
role="status" | ||
|
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.
We are not using any
children
prop here so we can removePropsWithChildren