-
Notifications
You must be signed in to change notification settings - Fork 937
Change the primary UI font, darken the background, and show template icons for workspaces #3863
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
Conversation
I think this looks a bit nicer. It's pretty subjective, but right now we sit in-between a light and a dark mode, but more on the dark side. This essentially transforms us into a dark mode.
I think the background is pretty dark, but is an improvement over the current theme. |
My PR is in now! I think a darker background will make it easier to get good color contrast. But I think some of the color contrast might be a little too strong now - what do you think about changing the |
@presleyp I think there are too many changes on Chromatic to go one by one, are you ok if we just accept all this time? |
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.
If you think that is ok, I'm good with that. Maybe would be worth seeing what it looks like with the @presleyp suggestion.
@BrunoQuaresma yeah I looked at chromatic and found it most helpful to look at the pages, not the individual components. But we want to make sure that the next PR doesn't have to review changes left over from this one. Is there a shortcut we can take? |
<PageHeaderSubtitle>{workspace.owner_name}</PageHeaderSubtitle> | ||
<Box display="flex"> | ||
{hasTemplateIcon && ( | ||
<img alt="" src={workspace.template_icon} className={styles.templateIcon} /> |
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 for now it's okay to use a generic alt string
<img alt="" src={workspace.template_icon} className={styles.templateIcon} /> | |
<img alt="Template icon" src={workspace.template_icon} className={styles.templateIcon} /> |
@presleyp yes. You can accept the build instead of a single component. |
@presleyp I swapped em! |
The title mentions it all! Check out storybook to see the changes.
Why change the font family?
Our UI looked very skunkworks. By adjusting the primary font to Inter, it becomes a lot more readable. This also aligns with more Material UI defaults, which is healthy for the codebase!
Why darken the background?
The dashboard was a light-dark mode, which put it in a weird spot. It wasn't very dark but wasn't very light. This adjusts the shades by 2, which makes the whole UI look a bit sharper from my perspective.
Why add icons on the workspaces page?
This change could be done in a separate PR, but I think it sharpens the UI visually. It's odd to have a primary UI indicator inset on the second column.