Skip to content

improve design system scss#10193

Merged
brad-decker merged 1 commit intodevelopfrom
improve-design-system-scss
Jan 19, 2021
Merged

improve design system scss#10193
brad-decker merged 1 commit intodevelopfrom
improve-design-system-scss

Conversation

@brad-decker
Copy link
Copy Markdown
Contributor

@brad-decker brad-decker commented Jan 14, 2021

Changes

  1. Implements new design system colors per figma
  2. Sets up an scss map for colors and typography variants
  3. Uses typography variants to reduce code repetition in mixins
  4. Adds a 'typography' mixin that takes a variant by name from the map and generates styles.
  5. Keeps the named typography variants for ease of use and backward compatibility.
.some-class {
  @include H4;
}

versus

.some-class {
  @include typography(h4);
}

Both are currently valid, the later is more explicit the former is easier to type.

Having the typography variant is useful for the Typography component I am working on because I can loop through variants thusly:

@each $variant in map-keys($typography-variants) {
  &--#{$variant} {
    @include typography($variant);
  }
}

If the variant map changes then the Typography component automatically supports it.

@brad-decker brad-decker force-pushed the improve-design-system-scss branch from 1ac0fa7 to 7ac9372 Compare January 14, 2021 22:28
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [7ac9372]
Page Load Metrics (543 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3110054157
domContentLoaded33180154113464
load33280254313364
domInteractive33180154113464

@brad-decker brad-decker marked this pull request as ready for review January 14, 2021 22:46
@brad-decker brad-decker requested a review from a team as a code owner January 14, 2021 22:46
@brad-decker brad-decker requested a review from danjm January 14, 2021 22:46
.nyc_output/**
.vscode/**
test/e2e/send-eth-with-private-key-test/**
*.scss
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... was prettier linting our scss before now? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was... and I think we should re-enable it once we tweak some stylelint settings?

It was breaking apart the src: lines in the typography file into multiple lines and stylelint would then complain about that.

@@ -0,0 +1,41 @@
export const COLORS = {
UI1: 'ui-1',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of these first 6 colours seems unclear to me (the UI# ones, and the black and white).

If we're going to try to make this more easily theme-able in the future, then "UI1" could be more descriptive. And "black" might be confusing if it's a dark theme, where we might want to assign a lighter colour to it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of the UI# is for exactly that, to make it so that the colors could be updated or even themed without changing the variable names or meaning. I do get your point about the black/white UI colors. I think we have a long way to go to get to a place where the color system supports easily swapping to a dark theme.

For example Material UI's palette color type

interface PaletteColor {
  light?: string;
  main: string;
  dark?: string;
  contrastText?: string;
}

particularly of interest is the contrastText which is the biggest barrier to entry with switching from light/dark themes -- finding the right accessible color for text when a given background color is applied.

As for black/white bases, they are usually added to color systems (including material-ui theme.white/theme.black) Also, in our case ui-black is not #000 and in different themes what is considered "white" and "black" may be variations of the color and not #fff / #000

Happy to discuss more and get design team into the conversation as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting - maybe this relates to conventions I just don't have any experience with then. For example if I were designing a new UI and I had to choose a background colour and a border colour, I'd have no idea whether 1 or 4 were more suitable. Presumably there must be something more we can say about when each of these could be used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are intended to be general-purpose colors for building UI and are used for font colors, borders, backgrounds, etc. If such a distinction could be made in usage I doubt a single name could cover it, or we'd end up with a very long list of color names -- some with identical values.

@rachelcope @cjeria @rickycodes @jakehaugen and I (if I'm forgetting anyone, my apologies) looked through some blog posts related to color systems at large organizations. In the end, we borrowed heavily from that research but took some liberties. This conversation predated the channel we created, and I'm going largely from memory here so I could be mistaken.

Gudahtt
Gudahtt previously approved these changes Jan 15, 2021
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@brad-decker brad-decker mentioned this pull request Jan 15, 2021
@brad-decker
Copy link
Copy Markdown
Contributor Author

@Gudahtt this has been updated to reflect a discrepancy @cjeria found with the Kovan color. The only place using the correct color was the networks setting tab. I've updated that and @cjeria has added the network-specific color swatches to the design system.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [b7b60c3]
Page Load Metrics (856 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaintNaNNaNNaNNaNNaN
domContentLoaded632112285410349
load633112385610249
domInteractive632112285310349

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@brad-decker brad-decker merged commit 4f66af6 into develop Jan 19, 2021
@brad-decker brad-decker deleted the improve-design-system-scss branch January 19, 2021 16:30
@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants