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

feat(tokens): Add box-shadow and line-height tokens #35

Merged
merged 6 commits into from Aug 16, 2019

Conversation

@SiTaggart
Copy link
Collaborator

SiTaggart commented Aug 14, 2019

Adding some missing tokens:

  • Focus ring box-shadow should ideally be the same style across all focusable elements. This is currently just using the one we use for Button (and the yet to be built new Input design by Sarah)
  • Line height tokens. These are tightly coupled to the font-size due to our use of unit less line-height values. I think this is fine. Using the font-size*1.5 to the nearest 4 rule that came out of the Typography work recently. @two24studios double check you're OK with the values for SendGrid based on that calculation
  • Fix some references to the old font size token naming convention
@now

This comment has been minimized.

Copy link

now bot commented Aug 14, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://paste-git-feat-token-additions.twilio-dsys.now.sh

type: number
props:
line-height-10:
value: "1.67" ##12px * 1.67 = 20px

This comment has been minimized.

Copy link
@TheSisb

TheSisb Aug 14, 2019

Collaborator

The ordering of these is weird. Is that because they map 1:1 to fontSizes?

This comment has been minimized.

Copy link
@TheSisb

TheSisb Aug 14, 2019

Collaborator

Would this be a breaking change?

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Aug 14, 2019

Author Collaborator

Yeah, because we use unitless values, we have to have a 1:1 mapping to the corresponding font size otherwise we won't get whole numbers for line-height. The line-height scale is based on font-size*1.5 rounded up to the nearest 4

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Aug 14, 2019

Author Collaborator

It's not a breaking change in that we never had any, and the values being replaced in the hard coded theme object were based on the old typographic scale, but the new ones aren't too far off

This comment has been minimized.

Copy link
@TheSisb

TheSisb Aug 15, 2019

Collaborator

We had some in paste-theme-tokens/src/default/index.ts

image

Which are being used already like in the website breadcrumb component

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Aug 16, 2019

Author Collaborator

Fixed up @TheSisb. Each font size now uses the correct line-height token in this branch.

@SiTaggart SiTaggart merged commit 706643b into master Aug 16, 2019
6 checks passed
6 checks passed
Semantic Pull Request ready to be squashed
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: prettier Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
now Deployment has completed
Details
@SiTaggart SiTaggart deleted the feat/token-additions branch Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.