Move color adjustments to separate config file #2356

martenbjork · 2019-10-24T18:57:17Z

🚩 This is a highly speculative, low-confidence suggestion. Consider it an idea.

WHY are these changes introduced?

With the new color system, ~10 colors are inputed and ~100 more are generated. These new colors are adjusted versions of the initial ones. These adjustments are hard-coded into utils.ts. It means that the color factory and the color blueprint are tightly coupled. This makes it hard to introduce new tooling around colors — if you want to play around with colors, your only option is to tweak utils.ts.

WHAT is this pull request doing?

It extracts the adjustments to a separate JSON (?) file.

Before:

actionOnDark: setLightness(color, 76),
actionOnLight: setLightness(color, 36),

After (now in colorAdjustments.json):

"actionOnDark": {
  "baseColor": "onSurface", // ACTION_ON_DARK is based on onSurface
  "light": {"lightness": 76}, // Apply this modification in light mode
  "dark": {"lightness": 76} // Apply this modification in dark mode
},
"actionOnLight": {
  "baseColor": "onSurface",
  "light": {"lightness": 36},
  "dark": {"lightness": 36}
},
...

Utils.ts reads this file, creates the corresponding colors and returns the same CSS Custom Properties as before.

WHAT can we do now?

We can build separate tooling that allows designers (like me or @jessebc) to play with these values. As long as the tool outputs the same JSON file as above, Polaris can use it and generate Custom Properties from it.

ghost · 2019-10-24T18:57:21Z

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

github-actions · 2019-10-24T18:59:52Z

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified	2
Files potentially affected	141

Details

All files potentially affected (total: 141)

📄 src/utilities/theme/color-adjustments.json (total: 141)

Files potentially affected (total: 141)

🧩 src/utilities/theme/utils.ts (total: 140)

Files potentially affected (total: 140)

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

Co-authored-by: Tim Layton <tim.layton@shopify.com>

tmlayton · 2019-10-24T23:10:24Z

As the aphorism goes "All problems in computer science can be solved by another level of indirection" 😄

The general idea I like, a data structure that represents each generated role and the manipulations that we map over based on the high level inputs. It’s more approachable for tweaking. The structure itself I would want to think about more. It is also tied to HSL which might change. That is just to say if this moves forward it depends on deciding the exact structure and color space.

kaelig · 2019-10-25T01:13:15Z

I 100% agree with Tim 👍

On a more general level, I'm not against the idea of storing data like this in a JSON file, as long as it comes with a schema to validate against.

In case you end up pursuing this route, here's what a schema looks like: https://github.com/Shopify/polaris-icons/blob/52b9ea555d2c8de7dda04d94bdea34ae78ea42ee/packages/polaris-icons-raw/tests/metadata-schema.json

We'd then be able to use a tool such as ajv to validate the JSON's file conformance with the schema.

BPScott · 2019-10-25T03:50:19Z

On a more general level, I'm not against the idea of storing data like this in a JSON file, as long as it comes with a schema to validate against.

Do we have any plans to consume this data in any language other than JS? If not then it might be easier to stick it in a js/ts file and validate it using TS types

kaelig · 2019-10-25T04:47:08Z

I was going to suggest JS or TS but this made me think JSON could be considered a better target:

We can build separate tooling that allows designers (like me or @jessebc) to play with these values. As long as the tool outputs the same JSON file as above, Polaris can use it and generate Custom Properties from it.

martenbjork · 2019-10-25T15:11:08Z

Thanks for the input, everyone!

The structure itself I would want to think about more. It is also tied to HSL which might change.

To me, the structure of the JSON file doesn't imply increased confidence a particular color space — it just says that the "knobs" that you can tweak (i.e. the API interface) will be lightness/saturation (and maybe hue). The factory is still free to use apply these changes in any color space (as PR #2361 demonstrates).

To rephrase the essence of this PR: It would be good to have an "API" for the color factory so that one can play with the adjustments outside the factory itself.

tmlayton · 2019-10-25T23:45:13Z

The structure and values of the data are tied to a color space, though. For example, lightness can’t be defined in the abstract because it translates to different colors in HSL, HSLuv, and Lab. So you need to know which knob you are turning.

tmlayton · 2019-10-25T23:50:05Z

src/utilities/theme/colorAdjustments.json

+  "branded": {"baseColor": "branded", "light": {}, "dark": {}},
+  "brandedAction": {
+    "baseColor": "branded",
+    "light": {"lightness": 25},


Just FYI this 25 lightness comes from manually pulling the lightness from the base jade color. In HSLuv, the lightness is 47.3.

Ah, that's interesting and weird. I stand corrected. Will read up on the color spaces and pick up the discussion next week!

tmlayton · 2019-10-25T23:52:56Z

Otherwise, yes an API for the factory is great. TBH we will probably go with HSL or HSLuv so the schema is likely fine 😄

This reverts commit 126fc15.

tmlayton · 2019-10-30T07:19:29Z

Besides for a rebase to sort out color roles that have since been added, this is good to go.

ghost · 2019-10-30T17:29:41Z

🎉 Thanks for your contribution to Polaris React!

Extract sub-color tweaks to JSON and parse them

25f7da8

martenbjork requested review from danrosenthal and tmlayton October 24, 2019 18:57

Remove vague words from tests

5ec529b

Remove awkward casing

158aaa2

martenbjork changed the title ~~Move color adjustments to JSON file~~ Move color adjustments to separate config file Oct 24, 2019

improve typing and clean up some linting errors

126fc15

Co-authored-by: Tim Layton <tim.layton@shopify.com>

tmlayton reviewed Oct 25, 2019

View reviewed changes

Tim Layton added 3 commits October 29, 2019 12:01

Revert "improve typing and clean up some linting errors"

386237b

This reverts commit 126fc15.

refactor to fix up types and general cleanup

6d29172

remove newline

00874bc

tmlayton added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Oct 30, 2019

Merge remote-tracking branch 'origin' into color-tokens-experiment

a652092

tmlayton approved these changes Oct 30, 2019

View reviewed changes

tmlayton merged commit 95a3450 into master Oct 30, 2019

tmlayton deleted the color-tokens-experiment branch October 30, 2019 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move color adjustments to separate config file #2356

Move color adjustments to separate config file #2356

martenbjork commented Oct 24, 2019 •

edited

Loading

ghost commented Oct 24, 2019

github-actions bot commented Oct 24, 2019 •

edited

Loading

Files potentially affected (total: 141)

Files potentially affected (total: 140)

tmlayton commented Oct 24, 2019

kaelig commented Oct 25, 2019

BPScott commented Oct 25, 2019

kaelig commented Oct 25, 2019

martenbjork commented Oct 25, 2019 •

edited

Loading

tmlayton commented Oct 25, 2019

tmlayton Oct 25, 2019

martenbjork Oct 25, 2019

tmlayton commented Oct 25, 2019

tmlayton commented Oct 30, 2019

ghost commented Oct 30, 2019

Move color adjustments to separate config file #2356

Move color adjustments to separate config file #2356

Conversation

martenbjork commented Oct 24, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

WHAT can we do now?

ghost commented Oct 24, 2019

github-actions bot commented Oct 24, 2019 • edited Loading

Details

Files potentially affected (total: 141)

Files potentially affected (total: 140)

tmlayton commented Oct 24, 2019

kaelig commented Oct 25, 2019

BPScott commented Oct 25, 2019

kaelig commented Oct 25, 2019

martenbjork commented Oct 25, 2019 • edited Loading

tmlayton commented Oct 25, 2019

tmlayton Oct 25, 2019

Choose a reason for hiding this comment

martenbjork Oct 25, 2019

Choose a reason for hiding this comment

tmlayton commented Oct 25, 2019

tmlayton commented Oct 30, 2019

ghost commented Oct 30, 2019

martenbjork commented Oct 24, 2019 •

edited

Loading

github-actions bot commented Oct 24, 2019 •

edited

Loading

martenbjork commented Oct 25, 2019 •

edited

Loading