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.

Sign up for GitHub

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

Jump to bottom

feat(design): implementation for style-dictionary #2807

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(design): implementation for style-dictionary #2807

wants to merge 1 commit into from
+3,519 −437

Conversation

kpanot
Copy link
Contributor

@kpanot kpanot commented Feb 6, 2025 •
edited
Loading

Proposed change

The purpose of this PR is to expose the @o3r/design features as style-dictionary modules.

Todo

  • Implement Metadata Format
  • Unit Tests
  • Integration Tests
  • Exposed code documentation
  • Doc: listing the modules
  • Doc: Get Started guide
  • Doc: Update Showcase Design Token Page
  • Angular builder options to switch implementation
  • ng-add schematics
  • Expose Otter style transform-group
  • Migration of Showcase

Related issues

- No issue associated -

@nx-cloud Nx Cloud
Copy link

nx-cloud bot commented Feb 6, 2025 •
edited
Loading

View your CI Pipeline Execution ↗ for commit 8da2421.

Command Status Duration Result
nx run-many --target=test-int ✅ Succeeded 1h 2m 18s View ↗
nx run-many --target=test-e2e ✅ Succeeded 8m 44s View ↗
nx run-many --target=build --projects=eslint-pl... ✅ Succeeded <1s View ↗
nx run-many --target=publish --nx-bail --userco... ✅ Succeeded 31s View ↗
nx run-many --target=prepare-publish --exclude-... ✅ Succeeded 1m 21s View ↗
nx run-many --target=build,build-swagger ✅ Succeeded 15m 2s View ↗
nx affected --target=test --collectCoverage ✅ Succeeded 10m 33s View ↗
nx affected --target=lint ✅ Succeeded 14m 39s View ↗
Additional runs (3) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2025-02-27 03:11:15 UTC

@github-actions github-actions bot added enhancement New feature or request project:@o3r/design labels Feb 6, 2025
@codecov Codecov
Copy link

codecov bot commented Feb 6, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.76%. Comparing base (28de61b) to head (8da2421).
Report is 8 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kpanot kpanot force-pushed the feature/token-transform branch 3 times, most recently from 2746fa8 to 2bcaba6 Compare February 10, 2025 05:06
@kpanot kpanot force-pushed the feature/token-transform branch 2 times, most recently from 3b74582 to c1a5710 Compare February 13, 2025 07:50
@kpanot kpanot force-pushed the feature/token-transform branch 5 times, most recently from 4d93534 to 81f3f8b Compare February 14, 2025 02:36
@kpanot kpanot force-pushed the feature/token-transform branch from 81f3f8b to 845f153 Compare February 14, 2025 03:09
@kpanot kpanot force-pushed the feature/token-transform branch 9 times, most recently from 7380b7a to 7b72471 Compare February 17, 2025 07:40
@kpanot kpanot marked this pull request as ready for review February 17, 2025 07:49
@kpanot kpanot requested a review from a team as a code owner February 17, 2025 07:49
@kpanot kpanot closed this Feb 20, 2025
@kpanot kpanot force-pushed the feature/token-transform branch from b59aa4e to 1fcb1ad Compare February 20, 2025 16:10
@kpanot kpanot reopened this Feb 20, 2025
@kpanot kpanot force-pushed the feature/token-transform branch 2 times, most recently from 117bbb5 to 29bdbb0 Compare February 20, 2025 16:20
vscaiceanu-1a
vscaiceanu-1a reviewed Feb 20, 2025
View reviewed changes
@kpanot kpanot force-pushed the feature/token-transform branch 4 times, most recently from 3892851 to 5e67bd4 Compare February 20, 2025 18:35
cpaulve-1A
cpaulve-1A reviewed Feb 21, 2025
View reviewed changes
packages/@o3r/style-dictionary/src/formats/css.format.mts
usesDtcg
});

const replacePrivateTokenReferences = (token: TransformedToken, strValue: string): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a unit test covering private references?

cpaulve-1A
cpaulve-1A reviewed Feb 21, 2025
View reviewed changes
packages/@o3r/style-dictionary/src/formats/metadata.format.spec.ts
beforeEach(() => jest.clearAllMocks());

test('should have otter prefix', () => {
expect(metadataFormat.name).toMatch(new RegExp(`^${OTTER_NAME_PREFIX}`));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could have test with example of metadata generated?

cpaulve-1A
cpaulve-1A reviewed Feb 21, 2025
View reviewed changes
packages/@o3r/style-dictionary/src/transforms/ratio.transform.mts

const numberPossiblePattern = /^(.*?)\s*[a-z]*$/;

const applyRatio = (value: any, ratio: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

could we consider something like calc(var(--my-variable) * ratio) or we will never get this use case?

Copy link
Contributor Author

@kpanot kpanot Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Just to make sure we are aligned on the context (before answering :D):
The purpose of the transform functions are to transform the value of a token before being generated to CSS.
A token can have different types including number, dimension, string, etc... with an associated value based on its type.
A token can actually contain CSS rules (or CSS function call) but in this case it will not be interpreted and the token should be flagged as string.

So in your case, if the token as the value calc(var(--my-variable) * ratio) the ratio will not be apply to it.

If you are talking about the final CSS generated, and you prefer to have --new-variable: calc(var(--my-variable) * ratio) instead of the current --my-variable: <value * ratio> then it should be handle by preprocessor (because it would request the creation of a new token) and this transform should not be used.
(This feature has never been requested so this preprocessor does not exist today :))

cpaulve-1A
cpaulve-1A reviewed Feb 21, 2025
View reviewed changes
packages/@o3r/style-dictionary/src/preprocessors/extensions.preprocessor.mts
return getNode(ext[node], tail);
};

const convertOtterNotationToToken = (token: TransformedToken): TransformedToken => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this method meant to do?
from what I understand token.themeable is meant to add the !default modifier, is it what o3rExpectOverride will do?
Should we add this in the documentation?

Copy link
Contributor Author

@kpanot kpanot Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is a bit more than !default but yes.
The purpose of this function is to convert the properties we have on the $extensions (following DT standard) to the feature existing on Style Dictionary (not part of the standard).

Today the only case we have are themeable and private.
As you mentioned, themeable is resulting to !default suffix in Sass but it can be other annotation to other languages (it has not effect on CSS Variable).

I added comment to the function to give short explanation.

@kpanot kpanot force-pushed the feature/token-transform branch 5 times, most recently from 549f010 to b812152 Compare February 25, 2025 04:35
vscaiceanu-1a
vscaiceanu-1a reviewed Feb 25, 2025
View reviewed changes
packages/@o3r/style-dictionary/schematics/ng-add/templates/config.mjs.template Outdated
Copy link
Member

Choose a reason for hiding this comment

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

config.mjs seems too generic. Can this file have a more specific name?

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 is the standard name from Style Dictionary, cf. https://styledictionary.com/getting-started/using_the_cli/#build

sdo-1A
sdo-1A reviewed Feb 26, 2025
View reviewed changes
@kpanot
feat: implement design token tooling via Style Dictionary
8da2421 
deprecate: target file listing interface to migrate to single map
docs: provide readme regarding Style Dictionary usage
@kpanot kpanot force-pushed the feature/token-transform branch from b812152 to 8da2421 Compare February 27, 2025 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthieu-crouzet matthieu-crouzet matthieu-crouzet left review comments

@vscaiceanu-1a vscaiceanu-1a vscaiceanu-1a left review comments

@fpaul-1A fpaul-1A fpaul-1A left review comments

@cpaulve-1A cpaulve-1A cpaulve-1A left review comments

@sdo-1A sdo-1A sdo-1A left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
deprecate The issue or pull request involves code deprecation documentation Improvements or additions to documentation enhancement New feature or request project:@ama-sdk/schematics project:@o3r/core project:@o3r/design project:@o3r/eslint-config project:@o3r/eslint-config-otter project:@o3r/pipeline project:@o3r/showcase project:@o3r/style-dictionary project:@o3r/workspace project:@o3r-training/training-tools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kpanot @matthieu-crouzet @vscaiceanu-1a @fpaul-1A @cpaulve-1A @sdo-1A