-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Anchor FM Theme Font Pairings #48661
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~61 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
24a6f52
to
f8aa5f1
Compare
I think the style preview of the Raleway / Cabin pair is reversed? I was expecting "Raleway" to be bolder than "Cabin": I see the appropriate I guess we need to consider the Anchor specific font pairings in one of these places: wp-calypso/client/landing/gutenboarding/onboarding-block/style-preview/preview.tsx Line 23 in 0ff4ccb
|
I was confused about the combination as well since we have Raleway and Cabin, and then Cabin and Raleway, but it was confirmed here -- 382-gh-Automattic/dotcom-manage#issuecomment-756272357 |
Checking the linked comment, I don't see anything about the font weight / boldness. Should the pairings look like
That's what I'd expect, but it's appearing like:
|
Oooh -- I misunderstood. Yes, you're right! |
f8aa5f1
to
54bd0e7
Compare
client/landing/gutenboarding/path.ts
Outdated
@@ -122,7 +122,6 @@ export function useOnboardingFlow(): string { | |||
} | |||
return FLOW_ID; | |||
} | |||
|
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.
Accidental?
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.
Hmm, I think so... VS Code UI makes it look like there's an extra space there with their annotations. Anyway, added it back and also removed the ANCHOR_FM_FLOW_ID
constant as promised!
@ianstewart Can you confirm for us that this is what we want: both font pairings available for both Anchor themes? And keeping the total pairings at 7 (five original plus the two new ones)? I assumed regardless of how many we offer specific to Anchor, we'd keep the total at five. |
@kwight that should work cc @sfougnier @ollierozdarz |
This is ready for review! |
0f860ff
to
c60fdd7
Compare
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 we're good!
c60fdd7
to
967ccfc
Compare
967ccfc
to
669776f
Compare
b2ac5a8
to
2a5a54c
Compare
Changes proposed in this Pull Request
Testing instructions
Screenshots
Fixes 382-gh-Automattic/dotcom-manage