-
Notifications
You must be signed in to change notification settings - Fork 1.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
Add new marketing type scale #1379
Conversation
🦋 Changeset detectedLatest commit: 22a98b5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…teps are identical)
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.
Generally LGTM. Just a couple open questions:
Should the variables like $mktg-h-size-0
be suffixed with -mktg
👉 $h-size-0-mktg
so it matches the utility classes. I kinda like the mktg
as a prefix more, but that could be a separate effort to change it everywhere.
These pairings are then used to define the responsive utility classes. Here, the first number in each list is desktop, second is tablet, and third is mobile:
Should the order be switched to be "mobile first" and follow the sm
-> md
-> lg
breakpoints order?
$mktg-header-spacing-large: -0.03em !default;
Is this optimized for Alliance to tweak its kerning or is it something we also should keep for the system fonts?
@simurai It is optimized for Alliance, but maybe this also makes sense for the system fonts… @trosage @ajashams do you want to weigh in here? 🙏 If this letter spacing doesn't make sense for the system fonts we could default this variable to a no-change value, and then change this value as we change the font to
Yeah that makes sense, I'll reverse it 🙏 the loop currently using these numbers/indexes in reverse is kind of weird (Edit: ✅ fixed below)
My thinking was to align this with the naming scheme that we used as we just added color variables in // -------- Marketing --------
$mktg-blue-primary: #4969ed;
$mktg-blue-secondary: #3355e0;
$mktg-green-primary: #2ea44f;
$mktg-green-secondary: #22863a;
$mktg-purple-primary: #6f57ff;
$mktg-purple-secondary: #614eda; Currently it's a bit all over the place… we have e.g.:
Happy to switch these new vars to use a trailing |
… class (rely on inherited values)
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.
My thinking was to align this with the naming scheme that we used as we just added color variables in
css/primitives
Oh.. right. Already forgot about that. OK, sure, we can look into namespacing another time.
This PR implements the new marketing type scale, as outlined in Gloss.
.h0-mktg
through.h6-mktg
.h000-mktg
,.h00-mktg
, and.lead-mktg
has been deprecated.f0-mktg
through.f6-mktg
has been added for body contentAll of the utility classes are responsive, and are defined through an array of
font-size
/line-height
pairings, ranging from0
to10
for headers, and0-8
for body content:This is done to expose all of these values as variables, so that they can be overridden as easily as possible. These variables are then used to create a
map
offont-size
andline-height
pairings:These pairings are then used to define the responsive utility classes. Here, the first number in each list is desktop, second is tablet, and third is mobile:
We then loop through this map in
typography.scss
to create the utility classes (note that we check if sequential size steps are identical, and if they are, we skip defining the same styles again for the breakpoint that follows):We use the same technique for the
.fX-mktg
body content classes. The raw output as of now for both of these are:Preview, headers:
Preview, body content: