-
Notifications
You must be signed in to change notification settings - Fork 24
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
Pull in updated spacing and container tokens #4702
Conversation
…stem into update-color-names
…base to primitives
…n-system into standardize-file-names
🦋 Changeset detectedLatest commit: 5e46c19 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 |
✅ Deploy Preview for unrivaled-bublanina-3a9bae ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…hpe-design-system into update-spacing-container-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.
A couple questions, rest looks good.
"codeSyntax": {} | ||
} | ||
} | ||
}, | ||
"38400": { |
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.
The primitive token to value pattern seems to break here. If I understand how these primitive tokens are being built up, there is a ratio of 25
between the token and value (e.g. 600 / 25 = 24, 25600 / 25 = 1024).
However, 38400 & 76800 seem to break this pattern. 38400 / 25 = 1536, not 1152 as currently defined; 76800 / 25 = 3072, not 1536.
Do we need to:
- add 28800 --> 1152
- modify 38400 --> 1536
- add 51200 --> 2048
- modify 78600 --> 3072
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.
Good catch. Yes 38400 --> 1536. I don't believe we have use for 1152, 2048, or 3072.
I've adjusted 38400, but not added the others.
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'm good with omitting those for now.
We don't need the 1152 because we are just hardcoding that in the HPE theme, correct?
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.
Exactly. 👍
Deploy Preview
What does this PR do?
Where should the reviewer start?
What testing has been done on this PR?
In addition to the feature you are implementing, have you checked the following:
General UX Checks
Accessibility Checks
Code Quality Checks
How should this be manually tested?
Any background context you want to provide?
What are the relevant issues?
Closes #4543
Screenshots (if appropriate)
Should this PR be mentioned in Design System updates?
Is this change backwards compatible or is it a breaking change?