refactor: redesign workspace status on workspaces table #17425
Conversation
There was a problem hiding this comment.
Looks good overall to me! Just had some questions/remarks
Also, I know that this is out of scope for the PR, but I can't help but feel like the border color should be slightly dimmer. Just bright enough to provide a boundary between the badge and the surrounding area's color, but still dim enough that it doesn't compete with the color of the badge text (which is the main part the user cares about)
| @@ -28,10 +28,12 @@ | |||
| --surface-grey: 240 5% 96%; | |||
| --surface-orange: 34 100% 92%; | |||
| --surface-sky: 201 94% 86%; | |||
| --surface-red: 0 93% 94%; | |||
There was a problem hiding this comment.
Is there a good way to get up to speed on how we have colors set up right now, especially for themes? This red is pretty bright and desaturated, and I'm wondering how it looks against the light theme
There was a problem hiding this comment.
Yes, you can check them in Figma - You may need to learn how to see the tokens. I'm not sure if we are following this, but in the past I think we decided to use TailwindCSS colors as reference.
| "border border-solid border-border-warning bg-surface-orange text-content-warning shadow", | ||
| destructive: | ||
| "border border-solid border-border-destructive bg-surface-red text-content-highlight-red shadow", |
There was a problem hiding this comment.
I want to say that border-solid is set automatically via border, but I guess it's good to be explicit here
There was a problem hiding this comment.
It is not 😢 . The reason is because we don't use the TailwindCSS pre-flight right now because of MUI - to avoid overlaping resets and styles.
|
|
||
| const variantByStatusType: Record< | ||
| GetWorkspaceDisplayStatusType, | ||
| StatusIndicatorProps["variant"] |
There was a problem hiding this comment.
Wondering if it's worth splitting off the variant property into a separate type, and exporting that instead. Chaining props like this feels like fragile, over-coupled dependencies
There was a problem hiding this comment.
Chaining props like this feels like fragile, over-coupled dependencies
I think this makes sense when the defined type is dependent on the another one. So, if I change the props onStatusIndicator it will be reflect on variantByStatusTypewithout any extra effort. Besides that, I think having something like:
type StatusIndicatorVariant = StatusIndicatorProps["variant"]It just works as an alias IMO.
There was a problem hiding this comment.
Putting more thoughts on it, I just think types dependency may warn us about components being too coupled, which is not always bad, but definitely something to evaluate.
|
|
||
| return ( | ||
| <TableCell> | ||
| <div className="flex flex-col"> |
There was a problem hiding this comment.
Is there a reason why this is a flex column? I would expect that when there isn't any gap set, you would get the same result if you were to remove the classes entirely
There was a problem hiding this comment.
That is true, let me check.
There was a problem hiding this comment.
Yes, it has to be because of the inline elements that are inside like the span.
| @@ -176,6 +176,7 @@ export const getDisplayWorkspaceStatus = ( | |||
| case undefined: | |||
| return { | |||
| text: "Loading", | |||
| type: "active", | |||
There was a problem hiding this comment.
I can't flag line 174, but could we get rid of the as const declarations, and add an explicit return type to the function? The current setup won't provide many safety nets if we accidentally add an extra field or make a typo
There was a problem hiding this comment.
It would also remove the need for the ReturnType-derived type on line 245
There was a problem hiding this comment.
I will take a look on those 👍
site/src/utils/workspace.tsx
Outdated
| @@ -307,3 +312,23 @@ const FALLBACK_ICON = "/icon/widgets.svg"; | |||
| export const getResourceIconPath = (resourceType: string): string => { | |||
| return BUILT_IN_ICON_PATHS[resourceType] ?? FALLBACK_ICON; | |||
| }; | |||
|
|
|||
| export const lastUsedMessage = (lastUsedAt: string | Date) => { | |||
There was a problem hiding this comment.
Can a return type be added here?
|
|
||
| if (t.isAfter(now.subtract(1, "hour"))) { | ||
| message = "Now"; | ||
| } else if (t.isAfter(now.subtract(3, "day"))) { |
There was a problem hiding this comment.
Am I crazy or do these middle branches not do anything since message already defaults to t.fromNow() on line 319?
There was a problem hiding this comment.
They do because the conditions are not using the message value to compare things... maybe I'm missing something 🤔 but in my tests and storybook things are working as expected - I guess haha
722e22f to
bbe9d8f
Compare
Closes #17310
Before:
After:
Notice!
xs. Since we reduced the line-height for thetext-xsto be 16px instead of 18px, having a smaller badge, reducing the vertical size and horizontal paddings, just worked better.