- Notifications
You must be signed in to change notification settings - Fork1.1k
refactor: redesign workspace status on workspaces table#17425
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
Conversation
buenos-nachos left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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)
| --surface-grey:2405%96%; | ||
| --surface-orange:34100%92%; | ||
| --surface-sky:20194%86%; | ||
| --surface-red:093%94%; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I want to say thatborder-solid is set automatically viaborder, but I guess it's good to be explicit here
BrunoQuaresmaApr 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
| constvariantByStatusType:Record< | ||
| GetWorkspaceDisplayStatusType, | ||
| StatusIndicatorProps["variant"] |
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.
Wondering if it's worth splitting off thevariant 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 onvariantByStatusTypewithout any extra effort. Besides that, I think having something like:
typeStatusIndicatorVariant=StatusIndicatorProps["variant"]
It just works as an alias IMO.
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.
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> | ||
| <divclassName="flex flex-col"> |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That is true, let me 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.
Yes, it has to be because of the inline elements that are inside like the span.
| caseundefined: | ||
| return{ | ||
| text:"Loading", | ||
| type:"active", |
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't flag line 174, but could we get rid of theas 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It would also remove the need for theReturnType-derived type on line 245
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 will take a look on those 👍
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.
Done ✅
site/src/utils/workspace.tsx Outdated
| returnBUILT_IN_ICON_PATHS[resourceType]??FALLBACK_ICON; | ||
| }; | ||
| exportconstlastUsedMessage=(lastUsedAt:string|Date)=>{ |
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.
Can a return type be added here?
| if(t.isAfter(now.subtract(1,"hour"))){ | ||
| message="Now"; | ||
| }elseif(t.isAfter(now.subtract(3,"day"))){ |
bcpeinhardtApr 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Am I crazy or do these middle branches not do anything since message already defaults tot.fromNow() on line 319?
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.
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 tobbe9d8fComparec8edada intomainUh oh!
There was an error while loading.Please reload this page.
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.