Skip to content
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

Normalize font styles to use typography mixins #9074

Closed
wants to merge 1 commit into from

Conversation

brad-decker
Copy link
Contributor

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

Attempt at normalizing all existing font styles to use our typography settings from the design system. I have added comments to all lines that I believe to result in changes to the presentation of the extension with screenshots of the changes

depends on:

  1. Add euclid fontface #9018
  2. Update css folder structure #9071
  3. Use mixins for typography instead of placeholder selectors #9072
  4. Update font family globally #9073

@brad-decker brad-decker force-pushed the set-font-family-globally branch from 144e401 to b37cf34 Compare July 27, 2020 15:30
@brad-decker brad-decker force-pushed the normalize-font-styles branch from 608ac46 to 51a842b Compare July 27, 2020 15:31
@metamaskbot
Copy link
Collaborator

Builds ready [51a842b]
Page Load Metrics (566 ± 30 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27373132
domContentLoaded3216575656330
load3226585666330
domInteractive3216565656330

@brad-decker
Copy link
Contributor Author

Update fonts

@brad-decker brad-decker force-pushed the normalize-font-styles branch from 51a842b to b2b83a7 Compare July 27, 2020 22:16
@metamaskbot
Copy link
Collaborator

Builds ready [b2b83a7]
Page Load Metrics (608 ± 35 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3010240157
domContentLoaded3557436077335
load3567456087335
domInteractive3547436077335

@brad-decker brad-decker force-pushed the set-font-family-globally branch from b37cf34 to 78d2678 Compare July 29, 2020 15:36
Base automatically changed from set-font-family-globally to develop July 29, 2020 20:04
@brad-decker brad-decker force-pushed the normalize-font-styles branch from b2b83a7 to c896550 Compare July 29, 2020 20:05
font-weight: normal;
font-size: 14px;
line-height: 20px;
@include Paragraph;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Believe we should be using font-size 16 for text, but this could also be a H6

Before
After

Copy link
Member

Choose a reason for hiding this comment

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

Both seem OK to me! @rachelcope , thoughts?

Choose a reason for hiding this comment

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

I think in the designs we've moved towards font-size 14 for subheader text. The font-size 16 looks a little big here. Is the default paragraph styles font-size 16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rachelcope paragraph styles match the figma design system at 1REM / 16px. Is this a subtitle or text? Cause its labeled as text, but happens right under a title so could be a subtitle too lol

@brad-decker brad-decker marked this pull request as ready for review July 29, 2020 20:14
@Gudahtt
Copy link
Member

Gudahtt commented Aug 6, 2020

Regarding the origin shown during the connect flow, I believe that size changed in an earlier PR. The current designs in Figma are inconsistent on size (it's 14px in some places, but 12px in most). I do prefer it at the larger size though, as we should be drawing user's attention to it, since it's the single trustworthy thing we know about the initiator.

@brad-decker brad-decker force-pushed the normalize-font-styles branch 2 times, most recently from f676f37 to a278b31 Compare August 11, 2020 17:12
@metamaskbot
Copy link
Collaborator

Builds ready [b4d5ba9]
Page Load Metrics (613 ± 27 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30433742
domContentLoaded3947136125727
load3967146135727
domInteractive3947136115727

@metamaskbot
Copy link
Collaborator

Builds ready [eb0974c]
Page Load Metrics (605 ± 56 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint308842157
domContentLoaded33573660311656
load33673860511656
domInteractive33573660211656

@brad-decker brad-decker force-pushed the normalize-font-styles branch from eb0974c to 27a440c Compare August 20, 2020 15:57
@metamaskbot
Copy link
Collaborator

Builds ready [27a440c]
Page Load Metrics (425 ± 63 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint288237115
domContentLoaded25760242313263
load25960342513263
domInteractive25760142313263

@metamaskbot
Copy link
Collaborator

Builds ready [a76ce2a]
Page Load Metrics (536 ± 78 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint38116682713
domContentLoaded34682053416278
load34782353616278
domInteractive34582053316278

@brad-decker
Copy link
Contributor Author

@rachelcope the font-weight of 500, medium, has almost no effect and font-weight 600 is almost the same as bold. This is font-weight 600.

image

Is this what it should look like?

@brad-decker
Copy link
Contributor Author

@rachelcope a new commit changes the button on asset pages font-weight to medium and fixes the sizing of the dapp URL. I'm waiting for some feedback from you on the account name bold issue and I am gonna do the typography stuff for Material UI in a separate PR to keep like things grouped together.

@rachelcope
Copy link

On the Account name, Font-weight 600 looks too bold to me. Is font-weight 500 slightly more bold than it was before? I would go with that.

Also, @Gudahtt had commented that the dapp url shouldn't be made smaller because that is the only place where a user can verify the origin. I forgot we had had that conversation before, but I will go with Mark's suggestion of not making it smaller.

Lastly, there are inconsistencies between what is outlined in the figma design system paragraph styles (16px) and what we actually use in the designs (I seem to always use 14px, because 16px seems too big for our screen real estate). I'm of the opinion that think we should update paragraph styles (across figma and css styles) to be 14px. @cjeria any thoughts about this?

@metamaskbot
Copy link
Collaborator

Builds ready [3f3d832]
Page Load Metrics (468 ± 74 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint319046189
domContentLoaded26776846615374
load26976946815374
domInteractive26776846615374

@brad-decker
Copy link
Contributor Author

@rachelcope if we make that change (paragraph font size) I'd like to do so in a new PR. The change implies a few different paths:

  1. Changing Paragraph to be size 14px.
  2. Finding all places in the code where we SHOULD be using font-size 14 instead of 16 and changing them.

That's going to be quite a lot to review and QA in addition to the QA efforts already done on this PR. I think an audit of where we are using font-size 16 would be warranted (its a lot) so we can see the potential impact of reducing to 14px... also not all places we're using font-size 16px should change. H5 is 16px.

This leads me to another point: It's very confusing to have two typography settings with the same font-size. Regardless of what you decide my opinion is you should eliminate one heading level:

H1
H2
H3
H4
H5
Paragraph (14px), replacing H6 (14px)
H6 - now 12px
H7 - Now 10 px
H8 - Now 8px

furthermore having H levels beyond 6 is confusing, at least to me due to the lack of an <h7> tag in HTML. Having H levels lower than paragraph also is slightly confusing.

H1 - 48px (new)
H2 - 40px
H3 - 32px
H4 - 24px
H5 - 18px
H6 - 16px
Body - 14px
Body 2 - 12px
Discliamer - 10 px
Label - Now 8px

is a stab in the dark of something with slightly more taxonomy and meaning, but I am open to whatever you think is best-- as long as we can do that and make those changes in a future PR because it will, again, drastically lengthen the QA window on this PR which was meant to avoid stylistic changes as much as possible.

@rachelcope
Copy link

@brad-decker makes sense to do the fonts in a different PR. I totally agree with the H6, H7, H8 not making sense. I like your suggestion. We might rethink if we need styles for 12px, 10px, and also 8px or if we could consolidate

@brad-decker
Copy link
Contributor Author

alright i reverted the change to the URL size on permissions screen

This is font-weight 500 on account name:
image

600 looks like the screenshot above and is not different than 700 :/

@rachelcope
Copy link

Font-weight 500 on LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [58723df]
Page Load Metrics (488 ± 62 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29102612713
domContentLoaded34780648612962
load34980748812962
domInteractive34780648612962

Attempt at normalizing all existing font styles to use our
typography settings
@brad-decker brad-decker force-pushed the normalize-font-styles branch from 58723df to c24244d Compare September 3, 2020 16:43
@brad-decker
Copy link
Contributor Author

@Gudahtt I'm not sure when we want this to land but it's ready for review. I think I responded to all your feedback.

@metamaskbot
Copy link
Collaborator

Builds ready [c24244d]
Page Load Metrics (516 ± 67 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint277838115
domContentLoaded31572951414067
load31773051614067
domInteractive31572851414067

@brad-decker
Copy link
Contributor Author

@github-actions github-actions bot locked and limited conversation to collaborators Oct 23, 2020
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