Skip to content

Updates Typography Variables and styles. #9017

Merged
brad-decker merged 5 commits intodevelopfrom
typography-vars
Jul 24, 2020
Merged

Updates Typography Variables and styles. #9017
brad-decker merged 5 commits intodevelopfrom
typography-vars

Conversation

@brad-decker
Copy link
Copy Markdown
Contributor

@brad-decker brad-decker commented Jul 16, 2020

@brad-decker brad-decker mentioned this pull request Jul 16, 2020
@brad-decker brad-decker force-pushed the typography-vars branch 3 times, most recently from ffcf6d3 to 06acab2 Compare July 21, 2020 16:53
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [5bce8b8]
Page Load Metrics (590 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint28105422110
domContentLoaded3687345888139
load3697355908139
domInteractive3687345888139

@brad-decker brad-decker marked this pull request as ready for review July 21, 2020 20:42
@brad-decker brad-decker requested a review from a team as a code owner July 21, 2020 20:42
@brad-decker
Copy link
Copy Markdown
Contributor Author

Update fonts

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.

Looks good overall - I had a couple of minor comments.

There remain some possible user-facing changes this could cause that are difficult to find by reading the styles, so... we might want to include a complete UI review as part of whichever release this lands in.

%H4 {
font-style: normal;
font-weight: normal;
font-size: 1.125rem;
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.

I noticed that the old placeholder selectors (e.g. %h4) also set the line-height. Was that omitted here intentionally?

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.

I added the line heights from the design system but I did not modify any setting of extra line-heights to help mitigate as many user-facing changes as possible in this PR.

}

%H4 {
font-style: normal;
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.

This was omitted from the old placeholder selectors. Was this addition intentional, and is this possible this could result in a user-facing change? 🤔

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.

I guess the same goes for font-family as well

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.

For the most part I tried to make sure that the changes I made resulted in very close to the original styling, but because of the nature of CSS, it's possible that due to cascades setting font-style: normal; might override a cascaded font-style from a parent. I think a UI review is probably in order as you mentioned in you review comment. All the typography styles in the design system specify font-style which is why I specified it here.

brad-decker and others added 4 commits July 24, 2020 10:27
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
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!

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [88bcded]
Page Load Metrics (564 ± 41 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint288537157
domContentLoaded3116785638641
load3126805648641
domInteractive3116785638641

@brad-decker brad-decker merged commit df8eb49 into develop Jul 24, 2020
@brad-decker brad-decker deleted the typography-vars branch July 24, 2020 16: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.

3 participants