-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat(typography): support typography theming #1664
Conversation
size-limit report 📦
|
Codecov Report
@@ Coverage Diff @@
## next #1664 +/- ##
=======================================
Coverage 93.23% 93.23%
=======================================
Files 222 222
Lines 2970 2970
Branches 721 721
=======================================
Hits 2769 2769
Misses 184 184
Partials 17 17 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
8f211a1
to
2175edb
Compare
"value": "var(--eds-font-weight-bold) var(--eds-font-size-40)/1.2 var(--eds-font-family-primary)" | ||
}, | ||
"mobile": { | ||
"value": "var(--eds-font-weight-bold) var(--eds-font-size-32)/1.25 var(--eds-font-family-primary)" |
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.
Now that this is separated, the component itself would have to handle breaks?
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.
correct. I marked a TODO for this, but I think that's probably the better way to do this anyway. Tokens shouldn't have logic like that IMO
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.
Makes sense to me
1dcac37
to
3519a3d
Compare
Changes to note in this PR:
|
5462488
to
77f9a1c
Compare
@@ -110,7 +110,7 @@ export const ProjectCard = ({ | |||
<Heading | |||
as={headingAs} | |||
className={styles['project-card__title']} | |||
size="body-sm" | |||
size="title-xs" |
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.
body-sm
may have changed in design while this was under way, so picked a near neighbor for this demo.
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.
Makes sense and body-sm
prob being removed as part of Heading ticket anyways
@@ -82,7 +82,8 @@ | |||
* The title communicates card organization. | |||
*/ | |||
.data-summary-card__title { | |||
@mixin eds-theme-typography-overline-sm; | |||
font: var(--eds-theme-typography-overline-sm); | |||
text-transform: uppercase; |
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.
There are a few examples of adding in text-transform
to some cases, because font
only supports CSS 2.1 values which doesn't include all-caps
sadly
- use font property instead of fixed mixins and property templates - add in tokens for each preset and usage - ensure mapping and snapshots match without change - replace documentation to explain font-based approach - fix and sync typography items not represented in code - replace legacy typography uses in components with near neighbors - update page example code - remove along example due to CSS variable mismatch
77f9a1c
to
5a7def6
Compare
This fiddle shows how mixing |
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.
Removed the along demo as along itself is now going to use EDS, has the updated theme, and keeping this as-is would show:
- an outdated theme
- improper overrides for font family values (manifesting from a limitation in how storybook fetches per-component styles and variables)
- page structure that may change in the future
This avoids us having to keep it in sync with any product code/branding.
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.
Think the wireframe page should be enough now?
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.
yah. eventually we will invest in a proper wireframe theme (possibly as the default), and use this demo to show how to prototype with EDS
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.
u m b r e l l a b r a n d
} | ||
|
||
.typography-usage--overline-sm { | ||
@mixin eds-typography-preset-012-bold; | ||
font: var(--eds-typography-preset-012); | ||
text-transform: uppercase; |
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.
Are we removing the larger weight treatment on the overline tokens?
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.
Actually, food for thought, perhaps overline can be a Text
component variant only and not a token itself since it also has text-transform
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.
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.
Think it's been out of sync for a while and never got noticed since this token prob has low usage... could consider even removing these
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.
@jinlee93 Ohh, yeah, all usages could be run thru Text
for clarity and simplicity. This would give us a nice way to avoid exposing this text-transform
thing.
Then we can greatly simplify these typography token pages and probably remove the css modules for those
} | ||
} | ||
} | ||
``` |
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.
Nice updates
"eds-typography-preset-011": "var(--eds-font-weight-medium) var(--eds-font-size-12)/1.333333333 var(--eds-font-family-primary)", | ||
"eds-typography-preset-011-bold": "var(--eds-font-weight-medium) var(--eds-font-size-12)/1.333333333 var(--eds-font-family-primary)", | ||
"eds-typography-preset-012": "var(--eds-font-weight-medium) var(--eds-font-size-11)/1.272727272 var(--eds-font-family-primary)", | ||
"eds-typography-preset-012-bold": "var(--eds-font-weight-medium) var(--eds-font-size-11)/1.272727272 var(--eds-font-family-primary)", |
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.
Although there are more tokens, I am pretty happy this exists now since it will ACTUALLy align our typography tokens closer to Figma and the designer's idea of type tokens
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 it looks good, great work! I have some thoughts about overline, but don't think it should be blocking
## [12.3.0](v12.2.0...v12.3.0) (2023-06-29) ### Features * **breadcrumbs:** allow custom separators ([#1663](#1663)) ([1fe0e6c](1fe0e6c)) * **button:** create t3 token for border radius ([#1665](#1665)) ([eadd005](eadd005)) * **icons:** make icons rounded outline and add send, mail ([#1669](#1669)) ([669d01c](669d01c)) * **tokens:** add t2 color bg-brand-primary-subtle ([#1668](#1668)) ([7137da6](7137da6)) * **typography:** support typography theming ([#1664](#1664)) ([3148751](3148751))
Summary:
Support the ability to theme typography presets that match each consuming project. This has the added benefit of also decoupling us from having to use the postCSS mixin stuff. More functionality with less complexity.
Test Plan:
edu-stack
ortraject
as a sanity check if changes affect build or deploy, or are breaking, such as token changes, widely used component updates, hooks changes, and major dependency upgrades.