Skip to content

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

Merged
merged 5 commits into from
Sep 6, 2022

Conversation

kylecarbs
Copy link
Member

The title mentions it all! Check out storybook to see the changes.

image

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.

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.
@kylecarbs kylecarbs requested a review from a team as a code owner September 4, 2022 18:37
@kylecarbs kylecarbs requested review from jsjoeio and removed request for a team September 4, 2022 18:37
@BrunoQuaresma
Copy link
Collaborator

I liked the changes. I just think the background is too dark if compared with the other component. What do you think?

I also would wait for this PR be merged from @presleyp #3842

@kylecarbs
Copy link
Member Author

I think the background is pretty dark, but is an improvement over the current theme.

@presleyp
Copy link
Contributor

presleyp commented Sep 6, 2022

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 colors.gray[3]s out for 4s in the palette?

@BrunoQuaresma
Copy link
Collaborator

@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?

Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a 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.

@presleyp
Copy link
Contributor

presleyp commented Sep 6, 2022

@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} />
Copy link
Contributor

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

Suggested change
<img alt="" src={workspace.template_icon} className={styles.templateIcon} />
<img alt="Template icon" src={workspace.template_icon} className={styles.templateIcon} />

@BrunoQuaresma
Copy link
Collaborator

@presleyp yes. You can accept the build instead of a single component.

Screen Shot 2022-09-06 at 13 37 24

@kylecarbs
Copy link
Member Author

@presleyp I swapped em!

@kylecarbs kylecarbs enabled auto-merge (squash) September 6, 2022 18:15
@kylecarbs kylecarbs merged commit 3264960 into main Sep 6, 2022
@kylecarbs kylecarbs deleted the darker branch September 6, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants