-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Feature] Add composite typography tokens #572
Conversation
…b.com:department-of-veterans-affairs/va-mobile-library into feature/495-narin-composite-typography-tokens
…b.com:department-of-veterans-affairs/va-mobile-library into feature/495-narin-composite-typography-tokens
…b.com:department-of-veterans-affairs/va-mobile-library into feature/495-narin-composite-typography-tokens
…b.com:department-of-veterans-affairs/va-mobile-library into feature/495-narin-composite-typography-tokens
@jessicawoodin Could you please review composite.json to check the values for accuracy? Thanks. |
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.
Three big things:
- A late change broke the TS output file which needs fixing
- The tacked-on AC about adding groupings to the Figma output didn't fall into this ticket as it didn't touch the Figma output, it should be propagated to a standalone ticket to ensure we don't forget that was still desired by design/Jessica
- I think all instances of
styles
andcomposite
should betypography
Searching typography gives the following from Wikipedia:
Typography is the art and technique of arranging type to make written language legible, readable and appealing when displayed. The arrangement of type involves selecting typefaces, point sizes, line lengths, line spacing, letter spacing, and spaces between pairs of letters.
which is exactly what these tokens are: an amalgamation of those base font building blocks into a coherent unit. While we don't imminently have any future sets of composite tokens I'm aware of, composite seems like a general classification of tokens made from other tokens while typography are the specific tokens that these composites are and these should be named accordingly.
Beyond that, just some other minor suggestions/comments/questions.
Co-authored-by: Tim R <TimRoe@users.noreply.github.com>
…phy declaration file to match .js file.
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.
Overall, this PR looks great to me. All of the values appear correct.
Narin and I re-visited the paragraph spacing tokens based on how things were going with the Text component. We decided to remove all paragraph spacing tokens, and only use spacing tokens instead. I will update Figma and the Design Tokens spreadsheet to reflect this change. In the meantime, attached is an image showing the new spacing tokens that should be used for marginBottom in the composite tokens.
Ready for second review. @jessicawoodin if you could please take a look at the updated Created separate ticket to look into combining figma export into single file: #577 |
Had some discussion with @jessicawoodin but may have misinterpreted the desired outcome. Will clarify in stand-up. |
…b.com:department-of-veterans-affairs/va-mobile-library into feature/495-narin-composite-typography-tokens
}, | ||
"attributes": { | ||
"category": "font", | ||
"type": "composite", |
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.
Should these also be updated to typography
?
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.
If so, think the filter (name) would also need to be updated.
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.
Approved with just one last optional minor comment/suggestion.
Description of Change
This PR creates a new
composite.json
file which contains composite tokens made up from our font family, line height, paragraph spacing, and letter spacing tokens.filter/font/composite-npm
filter for font tokens of typecomposite
typescript/es6-declarations/composite
format to generate TS declarations composite tokensvalue
s of object typetypeof
font.styles
. I realizestyles
can be a little confusing alongsidestyle.json
which contains Bold, Italic, etc. but that file is for Figma only and not exported to NPM. Open to different names for this export.Check npm for sample output
Testing Packages
Screenshots/Video
N/A
Testing
Tested by importing into components package.
PR Checklist
Code reviewer validation:
changelog
label applied if it's to be included in the changelogPublish
If changes warrant a new version per the versioning guidelines and the PR is approved and ready to merge:
main
into branchmain