Style dictionary shadow build #370

lukasoppermann · 2022-10-14T13:08:06Z

Summary

This adds the capability of building shadow tokens to our build flow.
As discussed in the accompanying issue we want shadows to be overridable per theme.

Tokens can now be overridden in two places e,g, for light: tokens/base/shadow/light.json5 and tokens/functional/shadow/light.json5.
All shadow properties are present in the files:

      "extraLarge": {
        "$value": {
          "color": "{base.shadow.color.extraLarge}",
          "x": "0px",
          "y": "12px",
          "blur": "28px"
        },
        "$type": "shadow"
      }

The shadow color is specified in the same file:

{
  "base": {
    "shadow": {
      "color": {
        "small": {
          "$value": "{base.color.scale.black}",
          "alpha": 0.96,
          "$type": "color"
        },
        // ...
      },
      "small": {
        "$value": {
          "color": "{base.shadow.color.small}",
          "x": "0px",
          "y": "1px",
          "blur": "0px"
        },
        "$type": "shadow"
      },
      // ...
    }
  }
}

Todo:

make shadows including ALL properties themable (e.g. dark modes may have outlines, etc.)
keep source shadow tokens contained in shadow folder
include theme colors at build time to be used during the build
include shadows in theme output file

List of notable changes:

added base and functional shadow tokens for dark and light mode
updated build flow to build tokens and export into theme fles, e.g. functional/themes/dark.css
updated output path for functional color tokens from functional/color/dark.css → functional/themes/dark.css
updated how base scales are build, now using tokens/functional/color/scales.json5 (output did not change)

What should reviewers focus on?

should shadow tokens be bundled in themes together with colors?
→ currently this is done for better DX, only one file needs to be included to get all theme related values. However we could also use a different approach to enable this. E.g. using @imports and exporting and index file. What is the desired way of dealing with this here?
does this allow the desired customization options?
do you see any issues either technical or something being hard to understand?

Steps to test:

Clone branch
yarn to install deps
yarn build:color-tokens

Input is in tokens, output is in tokens-v2-private/[css|sass|...]/functional/themes/[dark|light|...].*

Contributor checklist:

All new and existing CI checks pass
Tests prove that the feature works and covers both happy and unhappy paths
Any drop in coverage, breaking changes or regressions have been documented above
All developer debugging and non-functional logging has been removed
Related issues have been referenced in the PR description

Reviewer checklist:

Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
Check that tests prove the feature works and covers both happy and unhappy paths
Check that there aren't other open Pull Requests for the same update/change
Verify the design tokens changed in this PR are expected using the diffing results in this PR

changeset-bot · 2022-10-14T13:08:09Z

⚠️ No Changeset found

Latest commit: b85a209

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2022-10-14T13:08:20Z

Variables changed

No variables changed

github-actions · 2022-10-14T13:13:28Z

🟢 No design token changes found

simurai

Re. tokens that should have multiple shadows. Would that be possible with the current setup?

I assume we can't just add a second value like

"small": {
  "$value": {
    "color": "{base.shadow.color.small} {base.shadow.color.medium}",
    "x": "0px 0px",
    "y": "1px 3px",
    "blur": "0px 6px"
  },
  "$type": "shadow"
},

So maybe it can be composed with combining multiple tokens like:

"shadow-small-1": {
  "$value": {
    "color": "{base.shadow.color.small}",
    "x": "0px",
    "y": "1px",
    "blur": "0px"
  },
  "$type": "shadow"
},
"shadow-small-2": {
  "$value": {
    "color": "{base.shadow.color.medium}",
    "x": "0px",
    "y": "3px",
    "blur": "6px"
  },
  "$type": "shadow"
},
"shadow-small": {
  "$value": "{shadow-small-1} {shadow-small-2}",
  "$type": "shadow"
},

It seems there is still some discussions around adding shadows to the spec.

simurai · 2022-11-01T01:57:23Z

tokens/base/shadow/dark.json5

+      "color": {
+        "small": {
+          "$value": "{base.color.scale.black}",
+          "alpha": 0.96,
+          "$type": "color"
+        },
+        "medium": {
+          "$value": "{base.color.scale.gray.4}",
+          "alpha": 0.85,
+          "$type": "color"
+        },
+        "large": {
+          "$value": "{base.color.scale.gray.4}",
+          "alpha": 0.8,
+          "$type": "color"
+        },
+        "extraLarge": {
+          "$value": "{base.color.scale.gray.4}",
+          "alpha": 0.7,
+          "$type": "color"
+        },
+        "highlight": {
+          "$value": "{base.color.scale.white}",
+          "alpha": 0.25,
+          "$type": "color"
+        },
+        "inset": {
+          "$value": "{base.color.scale.gray.2}",
+          "alpha": 0,
+          "$type": "color"
+        }
+      },


Should these be moved to tokens/functional/color?

"small": { "$value": { - "color": "{base.shadow.color.small}", + "color": "{functional.color.shadow.small}",

I guess it's nice that all the shadow tokens are in the same place, but you could also argue that all colors should be under tokens/functional/color and not sprinkled around elsewhere. 🤔

I think it would be better to keep it with the shadows. Effectively this only adds an alpha value to an existing color.

Splitting it up would make working with shadows very complex, as you effectively define a shadow across two files per color mode (I think this gets to complex).

I this case I think the "structurally right" place is not worth the bad DX.

simurai · 2022-11-01T02:12:51Z

tokens/functional/shadow/light.json5

+{
+  shadow: {
+    small: {
+      $value: '{base.shadow.small}',


Do we need to have shadows defined in base AND functional? It seems for now the ones in functional are just aliasing the ones in base and there isn't really a difference. base.shadow.small = functional.shadow.small.

If we wanted to change it to just a single place, I guess we should only have shadows under functional since that's what people are supposed to used in components.

I am very open about this. We could define all shadows only as functional tokens. One could argue there are no base shadows.

I am open for either way.

One could argue there are no base shadows.

Yeah, IMO we don't need base tokens if we only alias them in functional. We could add shadows to base if we had many shadows like we do with color scales. E.g. base.shadow.[1-9]. But yes, maybe we can only add them if we need it one day.

lukasoppermann · 2022-11-01T08:01:53Z

Re. tokens that should have multiple shadows. Would that be possible with the current setup?

I assume we can't just add a second value like

"small": {
  "$value": {
    "color": "{base.shadow.color.small} {base.shadow.color.medium}",
    "x": "0px 0px",
    "y": "1px 3px",
    "blur": "0px 6px"
  },
  "$type": "shadow"
},

So maybe it can be composed with combining multiple tokens like:

"shadow-small-1": {
  "$value": {
    "color": "{base.shadow.color.small}",
    "x": "0px",
    "y": "1px",
    "blur": "0px"
  },
  "$type": "shadow"
},
"shadow-small-2": {
  "$value": {
    "color": "{base.shadow.color.medium}",
    "x": "0px",
    "y": "3px",
    "blur": "6px"
  },
  "$type": "shadow"
},
"shadow-small": {
  "$value": "{shadow-small-1} {shadow-small-2}",
  "$type": "shadow"
},

It seems there is still some discussions around adding shadows to the spec.

Multiple shadows is a bit limited in the specs, as you already mentioned. I would say we support it in a custom way until the specs figure out how to do it.

I would suggest to use an array of shadow value objects in the $value property.

{
  "shadow-small": {
    "$value": [
      {
        "color": "{base.shadow.color.small}",
        "x": "0px",
        "y": "1px",
        "blur": "0px"
      },
      {
        "color": "{base.shadow.color.medium}",
        "x": "0px",
        "y": "3px",
        "blur": "6px"
      }
    ],
    "$type": "shadow"
  }
}

I am not sure if this allows to use token references btu this may not be to critical for shadows.

lukasoppermann

@simurai I implemented multiple shadows now, using an array as I sketched out in the post above.

Another idea I am thinking about is to implement an alpha property as well. This would let us avoid using so many custom colors that are mostly one-time use.

{
  "shadow-small": {
    "$value": [
      {
        "color": "{base.color.grey.4}",
        "alpha": 0.85,
        "x": "0px",
        "y": "1px",
        "blur": "0px"
      },
      {
        "color": "{base.color.grey.2}",
        "alpha": 0.45,
        "x": "0px",
        "y": "3px",
        "blur": "6px"
      }
    ],
    "$type": "shadow"
  }
}

rezrah · 2022-11-14T10:10:39Z

config/filters/isShadow.test.ts

+  })
+
+  it('returns false if $type property is not `shadow`', () => {
+    expect(isShadow(getMockToken({$type: 'pumpkin'}))).toStrictEqual(false)


one day you're gonna have to tell me the story behind 'pumpkin'... 😝

config/transformers/shadowToCss.test.ts

config/transformers/shadowToCss.ts

rezrah · 2022-11-14T10:24:54Z

config/transformers/shadowToCss.test.ts

+        ]
+      })
+    ]
+    const expectedOutput = ['0px 2px 1px 0 #00000080, 0px 8px 16px 0 #22222233']


I would expect the output to be 0px 2px 1px 0 #00000080, 0px 8px 16px 0 #22222233 not an array.

The mapping of multiple input objects is a side-effect and isn't the responsibility of shadowToCss. Is it possible to keep this test pure and only test the main fn? Some maintainers - like myself - may use the tests as a way to understand what the functions do regarding I/O, but it's not immediately clear here.

The array is due to the way the test is written. Since we are only testing one value it is not needed and I have updated it.

config/transformers/shadowToCss.ts

rezrah · 2022-11-14T10:28:49Z

tokens/functional/shadow/dark.json5

+        offsetX: '0px',
+        offsetY: '16px',
+        blur: '32px',
+        spread: "0px"


is there a matcher on px here? Can you pass "0" with the same result?

Currently there is no check for the values of the properties. I wasn't sure if this would be an overkill. Do you think we should validate that every property is of the correct type?

I think I'd expect "0" to also be valid, so a guard (or warning) would be cool

Well, at the moment it IS valid. :D But of course it is not valid in w3c specs.

rezrah · 2022-11-14T10:30:10Z

tokens/functional/shadow/light.json5

+        $type: 'color'
+      },
+      semiBlack: {
+        "12": {


Should this be nested here? and what does "12" mean here?

Yes, this is a semiBlack with 12% opacity. But this is all WIP as the shadows need rework: #359

Co-authored-by: Rez <[email protected]>

* shadows with with referenced color to support color modes Co-authored-by: Rez <[email protected]>

lukasoppermann temporarily deployed to github-pages October 14, 2022 13:15 Inactive

lukasoppermann force-pushed the sd-shadows branch from 1cc1481 to 580e036 Compare October 24, 2022 07:52

lukasoppermann force-pushed the sd-shadows branch from 580e036 to ae95e4c Compare October 31, 2022 12:07

lukasoppermann temporarily deployed to github-pages October 31, 2022 12:10 Inactive

lukasoppermann temporarily deployed to github-pages October 31, 2022 12:48 Inactive

lukasoppermann requested review from langermank, rezrah, vdepizzol and simurai October 31, 2022 13:11

lukasoppermann temporarily deployed to github-pages October 31, 2022 13:12 Inactive

lukasoppermann force-pushed the sd-shadows branch from 4762485 to deaba90 Compare October 31, 2022 13:28

lukasoppermann marked this pull request as ready for review October 31, 2022 13:28

lukasoppermann requested a review from a team as a code owner October 31, 2022 13:28

lukasoppermann requested review from a team and siddharthkp October 31, 2022 13:28

lukasoppermann temporarily deployed to github-pages October 31, 2022 13:31 Inactive

simurai reviewed Nov 1, 2022

View reviewed changes

lukasoppermann commented Nov 7, 2022

View reviewed changes

lukasoppermann added 2 commits November 7, 2022 09:35

shadows with with referenced color to support color modes

14b5d16

include shadows in color build

25bcba0

lukasoppermann force-pushed the sd-shadows branch from deaba90 to 1bafd57 Compare November 7, 2022 08:40

fix types & deprecated

5052835

lukasoppermann force-pushed the sd-shadows branch from 1bafd57 to 5052835 Compare November 7, 2022 08:44

lukasoppermann temporarily deployed to github-pages November 7, 2022 08:47 Inactive

lukasoppermann temporarily deployed to github-pages November 7, 2022 15:04 Inactive

add multi layer shadows

41b2077

lukasoppermann force-pushed the sd-shadows branch from 04a4e4f to 41b2077 Compare November 8, 2022 10:56

lukasoppermann temporarily deployed to github-pages November 8, 2022 10:59 Inactive

simurai approved these changes Nov 9, 2022

View reviewed changes

rezrah mentioned this pull request Nov 11, 2022

JSON schema validation for color design tokens #390

Closed

3 tasks