Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

Remove reliance on the Invision DSM import scripts #66

Merged
merged 3 commits into from
Jun 20, 2019
Merged

Conversation

kaelig
Copy link
Contributor

@kaelig kaelig commented Jun 14, 2019

Not everyone at Shopify has access to Invision DSM, so let's manage color tokens directly in tokens/colors.yml instead.

@kaelig kaelig requested review from BPScott and alex-page June 14, 2019 21:36
@kaelig kaelig temporarily deployed to polaris-tokens-pr-66 June 14, 2019 21:36 Inactive
@kaelig kaelig temporarily deployed to polaris-tokens-pr-66 June 14, 2019 21:37 Inactive
@BPScott
Copy link
Member

BPScott commented Jun 14, 2019

A nice start!

Some extra steps that'd I'd like to see that will help simplify the colors.yml and make it a bit easier to edit:

  • Make values in colors.yml use the acutal colors instead of aliases (and then we can remove the whole aliases section at the top). So https://github.com/Shopify/polaris-tokens/pull/66/files#diff-331a823dcc461b72db5ed76f9928f5a5R62 changes from value: '{!color-purple-text}' to value: '#50495a'. Am I missing some other usage of aliases that means we can't do that?
  • formats/sketchpalette.js seems to be the only place where "meta.friendlyName" is used. Can we update that file to generate the friendly name based off the name of the token (lodash's startCase might be helpful), and then remove the friendly name from the metadata
  • gulpfile.js seems to be the onlyplace where "meta.filter" is used. You mentioned that the filter is generated from the color based on code from https://codepen.io/kaelig/full/jeObGP/. Would it be possible to extract the "guess the filter" code from that pen and use it within the gulpfile so we don't need to store the filter value, and then remove it from the the metadata.

@BPScott BPScott temporarily deployed to polaris-tokens-pr-66 June 14, 2019 22:49 Inactive
Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

It would be great to investigate the points @BPScott raised. I am not sure if they are blocking this PR and could be addressed in another one.

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

This is good to go.

Regarding my next steps:

  • Inlining the aliases changes the output so technically a breaking change (something for v3)
  • Deriving the filter is hard as the code used atm is not deterministic so we'd end up with new filter values every time we generate the dist folder which is no good

@BPScott BPScott mentioned this pull request Jun 20, 2019
2 tasks
@kaelig kaelig merged commit 98c3b6a into master Jun 20, 2019
@kaelig kaelig deleted the remove-dsm branch June 20, 2019 21:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants