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

fix(Typography): update type ramp to sync with design #1601

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

booc0mtaco
Copy link
Contributor

@booc0mtaco booc0mtaco commented Apr 25, 2023

Summary:

  • add additional precision to the coded type ramp, to help pixel sizes resolve to whole numbers in the CSS engine (prevents browsers from resolving 2- and 3-digit sizes as partial pixel values like 31.98px vs 32px)
  • sync up all font-size and line-height values between design and coded typography ramp
  • flag any items that need further attention
  • this is 1/n ... there may be future updates as we find any other discrepancies

Test Plan:

  • Manually tested my changes, and here are the details:
    • Review with design

@github-actions
Copy link

github-actions bot commented Apr 25, 2023

size-limit report 📦

Path Size
components 94.07 KB (0%)
styles 36.61 KB (+0.34% 🔺)

@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #1601 (8b0a958) into next (214cd88) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             next    #1601   +/-   ##
=======================================
  Coverage   91.46%   91.46%           
=======================================
  Files         279      279           
  Lines        4172     4172           
  Branches      784      784           
=======================================
  Hits         3816     3816           
+ Misses        354      330   -24     
- Partials        2       26   +24     

see 11 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -133,7 +133,7 @@
font-family: var(--eds-font-family-primary) $important;
font-weight: var(--eds-font-weight-medium) $important;
font-size: var(--eds-font-size-14) $important;
line-height: 1.57 $important;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This particular preset is very confused. In code, it is always 14/22. In design, it is sometimes 14/22 (14/24 for regular, and 14/22 for light and bold). In storybook it is always 14/24 (but 006-light is missing from the page). currently trying to clarify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing up to be based on 14/22 across the board

@booc0mtaco booc0mtaco changed the title fix(Typography): update line-height values to be more precise fix(Typography): update type ramp to sync with design Apr 26, 2023
@booc0mtaco booc0mtaco requested a review from a team April 26, 2023 16:04
@booc0mtaco
Copy link
Contributor Author

Screenshot 2023-04-26 at 11 06 39

@booc0mtaco
Copy link
Contributor Author

Waiting on some feedback for the build errors. 004-light is used in a few places, but is simultaneously not defined in the type ramp

@booc0mtaco booc0mtaco force-pushed the aholloway/EDS-926 branch 2 times, most recently from 26442b2 to 9420373 Compare April 26, 2023 22:43
- update type ramp line heights and font sizes to be more precise
- add missing values and update existing values
- update documentation to reflect values in figma
- remove unreferenced type ramp levels from storybook
- update the names of body typography presets (remove -text)
- update the names of caption typography presets (remove -text)
@@ -41,7 +41,7 @@
"value": "0.75rem"
},
"11": {
"value": "0.688rem"
"value": "0.6875rem"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding precision so that the pixel size doesn't include any partial pixels

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow, our process won't take changes like this, and generate other diffs to be committed (re: #1608)

@@ -40,31 +40,32 @@
@mixin eds-typography-preset-009-bold $important;
}

@define-mixin eds-theme-typography-body-text-lg $important {
@define-mixin eds-theme-typography-body-lg $important {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing -text from the mixin names to match design names in figma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding mappings to each weight instead of special code at the bottom. This object will come in use later to generate all the typography stuff, since there are several chunks that are duplicative

@booc0mtaco
Copy link
Contributor Author

@jeremiah-clothier this one grew a bit in scope. lemme know if you spot anything that needs clarification here

@booc0mtaco booc0mtaco merged commit be5b868 into next Apr 28, 2023
@booc0mtaco booc0mtaco deleted the aholloway/EDS-926 branch April 28, 2023 16:49
@booc0mtaco booc0mtaco mentioned this pull request May 2, 2023
booc0mtaco added a commit that referenced this pull request May 2, 2023
### [12.0.1](v12.0.0...v12.0.1) (2023-05-02)

### Bug Fixes

* polyfill for useid for react <18 ([#1595](#1595)) ([4d32194](4d32194))
* restore check for undefined any types ([#1600](#1600)) ([214cd88](214cd88))
* suppress warnings from useId imports and prop usage ([#1606](#1606)) ([c875d9d](c875d9d))
* **Typography:** update type ramp to sync with design  ([#1601](#1601)) ([be5b868](be5b868))
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.

1 participant