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

style(tokens): new token sets for core, semantic, and branding #318

Open
wants to merge 63 commits into
base: main
Choose a base branch
from

Conversation

ju-Skinner
Copy link
Collaborator

@ju-Skinner ju-Skinner commented Nov 18, 2024

Description

Adds semantic tokens to our Design System, leveraging both Token Studio and Style Dictionary

Fixes #(issue)

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • unit tests
  • e2e tests
  • accessibility tests
  • tested manually
  • other:

Checklist:

If not applicable, leave options unchecked.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes
  • Design has QA'ed and approved this PR

@github-actions github-actions bot added package: core Changes have been made to the Core package package: doc-componenets labels Nov 18, 2024
Copy link
Contributor

github-actions bot commented Nov 18, 2024

Dependency Review

✅ No vulnerabilities or license issues found.

Scanned Manifest Files

Copy link

netlify bot commented Nov 18, 2024

Deploy Preview for pine-design-system ready!

Name Link
🔨 Latest commit f222bf8
🔍 Latest deploy log https://app.netlify.com/sites/pine-design-system/deploys/6786d4defda59e000832fd63
😎 Deploy Preview https://deploy-preview-318--pine-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dev-kjbot and others added 21 commits January 14, 2025 14:02
they have been moved to the tokens/base folder
this is temporary because we'd like to have this as a separate library
ju-Skinner and others added 14 commits January 14, 2025 14:11
* fix: adjust color token values

* fix: adjust missed blue values

* style(tokens): new colors

---------

Co-authored-by: Julian Skinner <ju.skinner@gmail.com>
* style: updates some components with semantic tokens

* style(chip): update chip to use component semantic tokens

* style(input): create new component specific semantic tokens

created input.json for tokenstudio

updated both the themes and metadata.json files

---------

Co-authored-by: Julian Skinner <ju.skinner@gmail.com>
@ju-Skinner ju-Skinner changed the title [DNM} style(tokens): new token sets for core, semantic, and branding style(tokens): new token sets for core, semantic, and branding Jan 14, 2025
@ju-Skinner ju-Skinner marked this pull request as ready for review January 14, 2025 20:16
Copy link
Member

@pixelflips pixelflips left a comment

Choose a reason for hiding this comment

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

I have a few questions, but this is off to an awesome start. If anything is unclear or I am misunderstanding, jut let me know.


border-radius: var(--border-radius-default);
border-radius: var(--pine-dimension-xs);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a border-radius token?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See comment below

height: var(--sizing-input);
background-color: var(--pine-color-secondary);
border: var(--pine-border);
border-radius: var(--pine-dimension-2xs);
Copy link
Member

Choose a reason for hiding this comment

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

I am confused by the dimension token, is this replacing spacing? Also in some areas, a radius token (e.g. --pine-border-radius-round) is used and in others its dimensions (e.g. --pine-dimension-2xs).

Copy link
Collaborator Author

@ju-Skinner ju-Skinner Jan 14, 2025

Choose a reason for hiding this comment

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

@phillip yes, the dimension token now covers a few areas. See here for more details.

As for ...radius-round and radius-circle those are special cases so we called those out.

Comment on lines +169 to +170
padding-block: var(--pine-dimension-2xs);
padding-inline: var(--pine-dimension-025);
Copy link
Member

Choose a reason for hiding this comment

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

Are dimensions tshirt sizing or a numeric scale?

Copy link
Contributor

@anechol anechol Jan 14, 2025

Choose a reason for hiding this comment

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

T-shirt sizing is only for a small defined set of semantic tokens. Core tokens are still numeric. This particular token has no corresponding semantic token, so we defaulted back to core.

Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, but maybe we think about having these assigned to a component token, just so it's obvious it's a one-off.

/**
* Do not edit directly, this file was auto-generated.
*/

:root {
--pine-border-radius-0: 0px;
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to remove the unit (px) from the 0 values. 0 is usually unitless.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a style dictionary thing. We don't have control over that.

Copy link
Member

@pixelflips pixelflips left a comment

Choose a reason for hiding this comment

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

Thanks for the clarifications. LGTM!

Copy link
Contributor

@QuintonJason QuintonJason left a comment

Choose a reason for hiding this comment

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

Good to go. Great work!

Copy link
Contributor

@anechol anechol left a comment

Choose a reason for hiding this comment

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

Since we tackled this together, my approval is biased, but thanks for keeping this PR in line through all our changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core Changes have been made to the Core package package: doc-componenets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants