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(variables-scss): Formatting color tokens and support themes #DS-1461 #1624

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

curdaj
Copy link
Contributor

@curdaj curdaj commented Sep 9, 2024

Description

Additional context

Issue reference

Copy link

netlify bot commented Sep 9, 2024

Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit 1402cb5
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/66e2fbe8c1ca3d000877fc08
😎 Deploy Preview https://deploy-preview-1624--spirit-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 96 (🟢 up 1 from production)
Accessibility: 93 (no change from production)
Best Practices: 100 (no change from production)
SEO: 82 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

Copy link

netlify bot commented Sep 9, 2024

Deploy Preview for spirit-design-system-storybook canceled.

Name Link
🔨 Latest commit 1402cb5
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/66e2fbe8e2bc7d0008ec1264

@github-actions github-actions bot added the feature New feature or request label Sep 9, 2024
@curdaj curdaj marked this pull request as ready for review September 9, 2024 08:08
@coveralls
Copy link

coveralls commented Sep 9, 2024

Coverage Status

coverage: 78.696%. remained the same
when pulling 1402cb5 on feat/ds-1461-formatting-colors
into 01aa3cf on main.

@coveralls
Copy link

Coverage Status

coverage: 78.698%. remained the same
when pulling 63c80b3 on feat/ds-1461-formatting-colors
into 497de39 on main.

1 similar comment
@coveralls
Copy link

Coverage Status

coverage: 78.698%. remained the same
when pulling 63c80b3 on feat/ds-1461-formatting-colors
into 497de39 on main.

Copy link
Contributor

@pavelklibani pavelklibani left a comment

Choose a reason for hiding this comment

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

uf... good job xD

@literat
Copy link
Collaborator

literat commented Sep 9, 2024

Good job for solving all those parsing issues 👏 I know that some parts of the code are copied from previous exporters, but also think that we can do better. I have reserved some time to do the code review again together. There are a few blind spots for me, so let's discuss them :-) Thanks.

Copy link
Member

@crishpeen crishpeen left a comment

Choose a reason for hiding this comment

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

$focus-color: (
    basic: (
        color 01: $focus-basic-color-01,
    ),
) !default;

color 01 -> color-01


$gradients-color: (
    basic-overlay: (
        color 01: $gradients-basic-overlay-color-01,
        color 02: $gradients-basic-overlay-color-02,
    ),
) !default;

can this object be called gradient-colors? Or is it an exception?


You are missing the global colors map

$colors: (
    action: $action-colors,
    background: $background-colors,
    border: $border-colors,
    custom: $custom-colors,
    disabled: $disabled-colors,
    emotion: $emotion-colors,
    focus: $focus-colors,
    gradient: $gradient-colors,
    neutral: $neutral-colors,
    selected: $selected-colors,
    shadow: $shadow-colors,
    text: $text-colors,
) !default;

It links other maps

@curdaj curdaj force-pushed the feat/ds-1461-formatting-colors branch 2 times, most recently from 36b2095 to d519195 Compare September 10, 2024 11:42
@curdaj curdaj changed the title Feat(variables-scss): Formatting color tokens #DS-1461 Feat(variables-scss): Formatting color tokens and support themes #DS-1461 Sep 10, 2024
@curdaj
Copy link
Contributor Author

curdaj commented Sep 11, 2024

$focus-color: (
    basic: (
        color 01: $focus-basic-color-01,
    ),
) !default;

color 01 -> color-01

$gradients-color: (
    basic-overlay: (
        color 01: $gradients-basic-overlay-color-01,
        color 02: $gradients-basic-overlay-color-02,
    ),
) !default;

can this object be called gradient-colors? Or is it an exception?

You are missing the global colors map

$colors: (
    action: $action-colors,
    background: $background-colors,
    border: $border-colors,
    custom: $custom-colors,
    disabled: $disabled-colors,
    emotion: $emotion-colors,
    focus: $focus-colors,
    gradient: $gradient-colors,
    neutral: $neutral-colors,
    selected: $selected-colors,
    shadow: $shadow-colors,
    text: $text-colors,
) !default;

It links other maps

ad 1 - fixed in figma/supernova

ad 2 - in Supernova, there is a gradients name of the group, so its correct.
It should be renamed to gradient in Supernova.

ad 3 - added logic for global color object at the end of the file

@curdaj curdaj force-pushed the feat/ds-1461-formatting-colors branch 3 times, most recently from bc2032a to 5eb4144 Compare September 11, 2024 09:33
@curdaj curdaj requested a review from literat September 11, 2024 12:53
@curdaj curdaj force-pushed the feat/ds-1461-formatting-colors branch from 5e1aa86 to f988888 Compare September 12, 2024 13:48
@curdaj curdaj force-pushed the feat/ds-1461-formatting-colors branch 2 times, most recently from 2414485 to 15d94e6 Compare September 12, 2024 14:14
@curdaj curdaj force-pushed the feat/ds-1461-formatting-colors branch from 15d94e6 to 0cd6cbc Compare September 12, 2024 14:18
@curdaj curdaj force-pushed the feat/ds-1461-formatting-colors branch from 0cd6cbc to 1402cb5 Compare September 12, 2024 14:34
@curdaj curdaj merged commit 179aabc into main Sep 12, 2024
14 checks passed
@curdaj curdaj deleted the feat/ds-1461-formatting-colors branch September 12, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants