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

Update effectStylesToTokens.ts #12

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

JeroenRoodIHS
Copy link
Contributor

I haven't got any means to verify that my proposed changes work, but I feel like iterating over DROP_SHADOW or INNER_SHADOW effects is more suitable than picking the first occurrence of the effects array. Any shadow effect can have multiple individual shadows, instead of just one.

Please consider this as a suggestion. I would like to be able to include all of the shadow effect in the tokens file, rather than only the first occurrence. If you would like to dismiss this PR and create your own solution, or make changes to this PR, that would be fine by me. Just here to add my two cents. :)

I haven't got any means to verify that my proposed changes work, but I feel like iterating over DROP_SHADOW or INNER_SHADOW effects is more suitable than picking the first occurrence of the effects array. Any shadow effect can have multiple individual shadows, instead of just one.
@PavelLaptev
Copy link
Collaborator

@JeroenRoodIHS thank you! I'll check it this weekend 🙂

@JeroenRoodIHS
Copy link
Contributor Author

Thanks! 🙂 Let me know if you have any questions. Not sure if I am able to respond this weekend, but I'll check back when I can.

@PavelLaptev
Copy link
Collaborator

Thanks for the PR @JeroenRoodIHS
I would like to change it a little bit. According to this conversation design-tokens/community-group#100
It's better to pack into array only values, not the whole thing.
This one:

"new-sh": [
  {
    "$type": "shadow",
    "$value": {
      "inset": false,
      "color": "#e4505040",
      "offsetX": "0px",
      "offsetY": "4px",
      "blur": "54px",
      "spread": "0px"
    }
  },
  {
    "$type": "shadow",
    "$value": {
      "inset": false,
      "color": "#5b75ff40",
      "offsetX": "0px",
      "offsetY": "4px",
      "blur": "24px",
      "spread": "0px"
    }
  },
]

To this one:

"new-sh": {
  "$type": "shadow",
  "$value": [
    {
      "inset": false,
      "color": "#e4505040",
      "offsetX": "0px",
      "offsetY": "4px",
      "blur": "54px",
      "spread": "0px"
    },
    {
      "inset": false,
      "color": "#5b75ff40",
      "offsetX": "0px",
      "offsetY": "4px",
      "blur": "24px",
      "spread": "0px"
    },
    {
      "inset": false,
      "color": "#00000040",
      "offsetX": "0px",
      "offsetY": "4px",
      "blur": "4px",
      "spread": "0px"
    }
  ]
}

@JeroenRoodIHS
Copy link
Contributor Author

JeroenRoodIHS commented Jan 22, 2024

Thank you for the feedback! Apologies, it was an oversight of mine. I am fully aware of the DTCG's efforts (but only some of the discussions). Good suggestion.

Edit: I missed your changes, and assumed I would need to make changes myself. Thank you for your edits! I will look into those, to give a LGTM

Copy link
Contributor Author

@JeroenRoodIHS JeroenRoodIHS left a comment

Choose a reason for hiding this comment

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

I'm not a member of this repo, so I can merely comment on this update. I like it! It looks good to me. :)

@JeroenRoodIHS
Copy link
Contributor Author

JeroenRoodIHS commented Feb 5, 2024

Hi @PavelLaptev, is there something that I can or should do at this point? The change you suggested feels right to me, so I think the PR is good to go. Due to my limited experience with contributing to others' repos, I am not sure about what I need to do. I'd like to prevent holding back changes on my end, hence my question. Apologies for the inconvenience.😅

@PavelLaptev PavelLaptev merged commit 07a635f into tokens-bruecke:main Feb 5, 2024
@PavelLaptev
Copy link
Collaborator

@JeroenRoodIHS I just need to find time to merge it :-)

@JeroenRoodIHS
Copy link
Contributor Author

Alright! Thank you for your help! :)

P.S.: if I bothered you, apologies. It wasn't my intention to rush things.

@JeroenRoodIHS JeroenRoodIHS deleted the patch-1 branch February 5, 2024 08:55
@JeroenRoodIHS JeroenRoodIHS restored the patch-1 branch February 5, 2024 08:55
@PavelLaptev
Copy link
Collaborator

No prob :-) Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants