search

design-tokens / community-group

This is the official DTCG repository for the design tokens specification.
https://tr.designtokens.org
Other
1.53k stars 62 forks source link

Shadow type feedback #100

Closed c1rrus closed 1 month ago

c1rrus commented 2 years ago

Is the shadow composite type fit for purpose? Does it need to support multiple shadows, as some tools and platforms do?

Please share your feedback, thoughts and ideas in this issue.

drwpow commented 2 years ago

I like it. That makes logical sense to me. My only request, personally, is that arrays of shadow objects are accepted (example). IMO layering shadows still counts as “one shadow” and they are difficult/unwieldy to combine as separate tokens (i.e. they aren’t reusable because they depend on the precise position of other shadows in the array)

natemoo-re commented 2 years ago

Totally agree with @drwpow that supporting arrays of shadow objects is critical for my use cases.

Is the idea that color tokens also encode an alpha value? Personally I would love to see an optional opacity or alpha value on the shadow object so that color tokens wouldn't need to be created for each opacity.

WardMe commented 2 years ago

I don't see an inset property on shadows? How would we define box vs inner shadows?

TravisSpomer commented 2 years ago

Yes, that appears to be an oversight. Seems like an inset property would be required.

Chudesnov commented 2 years ago

I would love to see an optional opacity or alpha value on the shadow object so that color tokens wouldn't need to be created for each opacity.

@natemoo-re maybe I'm overgeneralising, but to me it seems that a color part of a shadow is still a color token (even though it's "anonymous" and probably should never be referenced by anyone from the outside), it seems less then ideal to have separate definition mechanisms for the same types of tokens in different contexts.

Maybe, instead of that, sub-properties for composite tokens' $value should allow plain values as a shorthand but generally expect an anonymous token definition ({ $value, etc... }); then opacity/alpha/mixing transformation could be defined locally under that anonymous token.

Here's an example of what I mean (based on a different but similar spec we've been working on for some time at our company):


{
  "color": {
    "accent": {
      "$type": "color",
      "$value": "#000000"
    }
  },
  "shadow-token": {
    // most Web-compatible implementations would need support for multi-layered shadows,
    // so this might be "shadowLayer" instead
    "$type": "shadow",
    "$value": {
      "color": {
        // in our case it's "$value": {"$ref": "color.accent"} instead
        "$value": "{color.accent}", 
        "$transforms": [
          // based on https://www.w3.org/TR/css-color-5/#relative-RGB
          { "$type": "rgb", args: [{"$name": "alpha", "$value": "88"}] } 
        ]
      },
      "offsetX": "0.5rem",
      "offsetY": "0.5rem",
      "blur": "1.5rem",
      "spread": "0rem"
    }
  }
}
CITguy commented 2 years ago

Akin to https://github.com/design-tokens/community-group/issues/100#issuecomment-1037948510 and https://github.com/design-tokens/community-group/issues/100#issuecomment-1039804291, a boolean inset property is required to clarify whether a shadow should be internal vs external.

Figma provides two ways to define shadows (Drop Shadow and Inner Shadow):

Screen Shot 2022-06-06 at 3 37 41 PM

Inner Shadow corresponds to the use of the inset keyword for a CSS box-shadow property.

The equivalent tokens for the above shadows should be as follows:

{
  "dropShadow": {
    "$type": "shadow",
    "$value": {
      "offsetX": "0px",
      "offsetY": "0px",
      "blur": "2px",
      "spread": "4px",
      "color": "#cccccc"
    }
  },
  "innerShadow": {
    "$type": "shadow",
    "$value": {
      "offsetX": "0px",
      "offsetY": "0px",
      "blur": "2px",
      "spread": "4px",
      "color": "#cccccc",
      "inner": true
    }
  }
}

Resolution

Caveats

  1. Similar to the spread property, the inner property doesn't apply for CSS text-shadow.
CITguy commented 2 years ago

How would a CSS translation tool know if a $type: "shadow" token is a box-shadow or a text-shadow?

Flutter and CSS have different definitions for a shadow when applied in different contexts. I would assume that other environments would also have similar differentiation.

Text Shadow

Box Shadow

MatthiasDunker commented 2 years ago

This might be connected to https://github.com/design-tokens/community-group/issues/134 Maybe composite tokens are too specific and might exclude specific coding environments. Is it the task of tokens to take care of their usage? Should token types contain assumed context or semantics?

MatthiasDunker commented 2 years ago

Another thought: Should the standardization of design tokens empower (to build custom design systems) or limit down (to follow a given approach)? Just using primitive tokens leaves a lot of space for interpretation and thus causes a lot of work and vagueness for translation tools. Narrowing it down to composite tokens helps avoiding disambiguation, but on the other hand assumes how tokens should be used. And we can’t foresee future frameworks, design systems, whatever (maybe we don’t have to, because this will be the standard ?!).

Is there something in between? Like a name-spaced type set? like 

{
  "dropShadow": {
    "$type": "@web/shadow",
    "$value": {
      "offsetX": "0px",
      "offsetY": "0px",
      "blur": "2px",
      "spread": "4px",
      "color": "#cccccc"
    }
  }
}

or even an offer of multiple types? (thinking of multiple color spaces or ways of defining line-height (like in px, w/o unit or em....))

{
  "dropShadow1": [
    {
      "$type": "@web/shadow",
      "$value": {
        "offsetX": "0px",
        "offsetY": "0px",
        "blur": "2px",
        "spread": "4px",
        "color": "#cccccc"
      }
    },
    {
      "$type": "@flutter/shadow",
      "$value": {
        "blurRadius": 2,
        "spreadRadius": 4,
        "color": "#cccccc",
        "offset": [0, 0],
        "blurStyle": "normal"
      }
    }
  ]
}

Downside: this does not look like a standard, but like a collection of different platform definitions.

CITguy commented 2 years ago

My point was to show that the concept of a Text Shadow and Box Shadow (e.g., "drop shadow", "inner shadow", etc.) are present in multiple platforms. I was just providing examples of their implementation.

RobM-ADP commented 1 year ago

On the web, box-shadow is often used for things like focus rings for a couple of good reasons. You can have more than one of them and they can be animated. Sometimes though, in these cases, box-shadow is just the mechanism to implement what is essentially multiple strokes. It seems like having a "standard" way to describe multiple stroke/borders that is different than shadow would be necessary in addition to having the ability to define multiple shadows.

pascalduez commented 1 year ago

In addition to all other already mentioned shadow types should we also consider CSS filters drop-shadow()?
Seems like a valid way to add shadows to an element, helpful in some cases.
And obviously, does not have the same signature as box-shadow.

TravisSpomer commented 1 year ago

It seems like the spec as-is would already be perfect for drop-shadow()—the X-offset, Y-offset, blur radius, and color properties would translate 1:1 to drop-shadow(). Or are you proposing another example beyond box-shadow versus text-shadow of how a CSS translation tool might need additional information to decide which CSS property to output?

pascalduez commented 1 year ago

how a CSS translation tool might need additional information to decide which CSS property to output?

This is the heart of the issue I guess, and what I'm wondering too.

So let's say that with the current spec some of the keys are optional.

{
   // box-shadow intended
  "small": {
    "$type": "shadow",
    "$value": {
      "inset": true,
      "offsetX": "0px",
      "offsetY": "1px",
      "blur": "2px",
      "spread": "0px",
      "color": "#ccc"
    }
  },

  // box-shadow intended
  "small": {
    "$type": "shadow",
    "$value": {
      "offsetX": "0px",
      "offsetY": "1px",
      "blur": "2px",
      "spread": "0px",
      "color": "#ccc"
    }
  },

  // drop-shadow intended
  "small": {
    "$type": "shadow",
    "$value": {
      "offsetX": "0px",
      "offsetY": "1px",
      "blur": "2px",
      "color": "#ccc"
    }
  },

  // text-shadow intended
  "small": {
    "$type": "shadow",
    "$value": {
      "offsetX": "0px",
      "offsetY": "1px",
      "blur": "2px",
      "color": "#ccc"
    }
  }
}

If we only have one shadow $type, how would a translation tool be able to determine the output? Through $extensions?
In the box-shadow case missing keys could probably be filled with 0px, but feels a bit fragile, will most likely have a different final rendering, and doesn't cover all other formats. Sorry, bringing more questions here and not much solutions for the moment :sweat_smile:

lukasoppermann commented 1 year ago

I would love to see an optional opacity or alpha value on the shadow object so that color tokens wouldn't need to be created for each opacity.

@natemoo-re maybe I'm overgeneralising, but to me it seems that a color part of a shadow is still a color token (even though it's "anonymous" and probably should never be referenced by anyone from the outside), it seems less then ideal to have separate definition mechanisms for the same types of tokens in different contexts.

Maybe, instead of that, sub-properties for composite tokens' $value should allow plain values as a shorthand but generally expect an anonymous token definition ({ $value, etc... }); then opacity/alpha/mixing transformation could be defined locally under that anonymous token.

Here's an example of what I mean (based on a different but similar spec we've been working on for some time at our company):

{
  "color": {
    "accent": {
      "$type": "color",
      "$value": "#000000"
    }
  },
  "shadow-token": {
    // most Web-compatible implementations would need support for multi-layered shadows,
    // so this might be "shadowLayer" instead
    "$type": "shadow",
    "$value": {
      "color": {
        // in our case it's "$value": {"$ref": "color.accent"} instead
        "$value": "{color.accent}", 
        "$transforms": [
          // based on https://www.w3.org/TR/css-color-5/#relative-RGB
          { "$type": "rgb", args: [{"$name": "alpha", "$value": "88"}] } 
        ]
      },
      "offsetX": "0.5rem",
      "offsetY": "0.5rem",
      "blur": "1.5rem",
      "spread": "0rem"
    }
  }
}

I don't know how well this fits into the spec, but I love the idea.

With shadows there are typically a lot of one-off colors because you define different layers with different opacities for each shadow and if you add layers on every step of your shadow scale, the opacity values for colors often don't repeat.

Mix this with potentially color shadows, e.g. for danger and success and you get a lot of one-time use colors. Having a simple and easy way to define it in the shadow would be ideal.

However nesting $value objects in other $value objects may be complicated for parsing. So maybe it would be better to use the same transformation logic on colors and on shadows? So both can take a $transforms property? The problem with this is of course, what happens if you reference a color with transforms and add transformers on the shadow again? But maybe that is also not a problem.

lukasoppermann commented 1 year ago

Another problem that I notices is multi-layer shadows. In modern design most shadows are defined from multiple layers to get a better visual. However with the current spec it would be a very cumbersome work of defining multiple shadow layers and applying those after transformation:

{
  "shadows": {
    "sm": {
      "layer-01": //... shadow token,
      "layer-02": //... shadow token
    }
  }
}

And to apply in e.g. css:

button {
  box-shadow: var(--shadows-sm-layer-01), var(--shadows-sm-layer-01);
}

Not only is this more work for the end-users, it also means they need to know internal details of the shadows, to be able to combine the right layers.

I would love to see an option for an array of value groups for shadow tokens, so that the above would change to:

{
  "shadows": {
    "sm": {
      "$type": "shadow",
      "$value": [
        { // layer 01
          "color": "#cccccc66",
          "offsetX": "0px",
          "offsetY": "1px",
          "blur": "0px",
          "spread": "0px"
        },
        { // layer 02
          "color": "#cccccc33",
          "offsetX": "0px",
          "offsetY": "1px",
          "blur": "3px",
          "spread": "0px"
        }
      ]
    }
  }
}

And to apply in e.g. css:

button {
  box-shadow: var(--shadows-sm);
}
PavelLaptev commented 1 year ago

Agreed with @lukasoppermann Array type was made to declare collections and it logical to use it and easier to transform it later. Layer tokens don't need names in this case like "layer-1", "layer-2" etc.

acstll commented 1 year ago

Also agree with @lukasoppermann :+1:

I would love to see an option for an array of value groups for shadow tokens

c1rrus commented 1 year ago

Just been catching up on all this great feedback. Thanks y'all!

First off, I completely agree that shadow tokens should be able to have values that are an array (as suggested by @drwpow & @lukasoppermann) so that they can express layered shadows. My only question there is should the structure of a shadow $value always be an array (which could just contain a single element) or should it be object (as currently specced) or an array? I don't have strong feelings either way.

As for the inset stuff, I agree there should be a way to express that. However, I wonder if there's value in also retaining something like the current syntax for the purpose of defining "generic" shadows that are intended to be used both as inset and outset shadows on containers or as text shadows.

In that case one might argue that decision of how a shadow should be applied is separate from the definition of the shadow itself. So perhaps there needs to be an additional token type (shadowStyle perhaps?) for tokens that express a shadow and how it should be applied.

For example:

{
  "elevation-1": {
    "$type": "shadowStyle",
    "$value": {
      "shadow": {
        // this sub-value is either a shadow value or reference to a shadow token
        "color": "00000080",
        "offsetX": "0rem",
        "offsetY": "0.5rem",
        "blur": "1rem",
        "spread": "0rem"
      },
      "inset": false
    }
  },

  // example using a reference for the shadow
  "cut-out": {
    "$type": "shadowStyle",
    "$value": {
      "shadow": "{shadow-1}",
      "inset": true
    }
  },

  // a generic shadow (as per the current draft spec)
  "shadow-1": {
    "$type": "shadow",
    "$value": {
      "color": "12121240",
      "offsetX": "0rem",
      "offsetY": "0.5rem",
      "blur": "1rem",
      "spread": "0rem"
    }
  }
}

What do you think? Useful or overengineered/confusing? 😅

lukasoppermann commented 1 year ago

Shadow array

My only question there is should the structure of a shadow $value always be an array (which could just contain a single element) or should it be object (as currently specced) or an array? I don't have strong feelings either way.

I personally like the idea of always using an array as it makes the easier to understand and is always the same. However we do have cases like with fontFamily were $values can be string or array.

The benefit of keeping both possibilities would be that it may be easier for people who are new to the standard or don't know json so well.

While may here think design tokens will be always machine made, I disagree and see at least a few years of people (possible designers who don't really know code) tweaking tokens in json files.

Inset

As for the inset stuff, I agree there should be a way to express that. However, I wonder if there's value in also retaining something like the current syntax for the purpose of defining "generic" shadows that are intended to be used both as inset and outset shadows on containers or as text shadows.

Not sure about this. The one thing that bothers me is that the inset value changes the meaning of the x and why values. offsetX: 1px for inset: false means the shadow is at the bottom whereas on the inset: true it is on the top. (Yes, I know this makes sense as the imaginary light source stays the same but designers often look at this as a shadow on the top or bottom). This is why it may make sense to keep inset with the rest of the values. But I have no strong opinion here.

TravisSpomer commented 1 year ago

What do you think? Useful or overengineered/confusing? 😅

I've hardly ever used inset shadows so I'd defer to someone with more experience, but: it seems pretty optimistic to me that you'd commonly have two shadows with otherwise-identical properties but one's inset and one's regular—I imagine you'd want slightly different offsets and blurs even for shadows that would be used together. So my hunch is that it would just be added complexity for a pretty rare use case. (But if I'm wrong in that assumption and it is something people often do, your proposal seems like the right way to accomplish it.)

PavelLaptev commented 1 year ago

About inset. In specs, there is the spread property already which used not so often. So I think that inset also deserves a place in the specifications. I like this style when we have inset property inside the value. Just to keep it simple.

"elevation-1": {
    "$type": "shadow",
    "$value": {
        "color": "00000080",
        "offsetX": "0rem",
        "offsetY": "0.5rem",
        "blur": "1rem",
        "spread": "0rem",
        "inset": false
    }
  },

I don't think that even if inset changes the meaning of the x and y it could be a problem. Designers using graphic tool to adjust shadows, so they see a result. Also, don't think that someone will manually change inset to true also without checking it.

PavelLaptev commented 1 year ago

Another question about multiple shadows within one.

CSS box-shadow: inset 0 2px 0px #dcffa6, 0 2px 5px #000;

SwiftUI Circle().fill(Color.green).frame(width: 300, height: 300).shadow(color: .black, radius: 10).shadow(color: .red, radius: 40).shadow(color: .green, radius: 10)

In Figma you can create styles with multiple effects.

joshferrell commented 11 months ago

Inset can be used for dark mode shadows shadows to provide some extra contrast.

https://codyhouse.co/nuggets/beautiful-css-shadows

In that example the shadow looks something like this:

inset 0 0 0.5px 1px hsla(0, 0%,  100%, 0.075),
0 0 0 1px hsla(0, 0%, 0%, 0.05),
0 0.3px 0.4px hsla(0, 0%, 0%, 0.02),
0 0.9px 1.5px hsla(0, 0%, 0%, 0.045),
0 3.5px 6px hsla(0, 0%, 0%, 0.09);

applying a global inset doesn't work in this case, instead it should be at the object level in the array.

I feel like cobalt-ui's implementation is very straightforward with:

"$value": [
  {
    "inset": true,
    "offsetX": "0px",
    "offsetY": "0.5px",
    "blur": "0px",
    "color": "rgba(255, 255, 255, 0.1)"
  },
  {
    "inset": true,
    "offsetX": "0px",
    "offsetY": "0px",
    "blur": "0.5px",
    "color": "rgba(255, 255, 255, 0.3)"
  },
  {
    "offsetX": "0px",
    "offsetY": "0px",
    "blur": "0.5px",
    "color": "rgba(0, 0, 0, 0.5)"
  },
  {
    "offsetX": "0px",
    "offsetY": "1px",
    "blur": "3px",
    "color": "rgba(0, 0, 0, 0.4)"
  }
]

For consistency sake, having it always be an array makes most sense to me, though it could support both. Having both single object and array creates some added complexity for library maintainers when parsing it, but not too much imo.

CITguy commented 11 months ago

For consistency sake, having it always be an array makes most sense to me, though it could support both. Having both single object and array creates some added complexity for library maintainers when parsing it, but not too much imo.

The internal parser I wrote for our design tokens just coerces every value into an array, regardless of token type. Doing this allows my translation logic to be more robust, because it can handle any number of values that are thrown at it. The tricky part was figuring out how to resolve various configurations that contain aliases. The short answer is that we prohibit aliases that resolve to arrays, because it's unclear if you'd want to splice, concatenate, or nest arrays within arrays. By prohibiting aliases to array values, we can ensure that the resolved value of the parent token is always a 1-dimensional array of tokens.

This deviation from the spec is acceptable to us because my team is the only one actually using the parser directly. Given that we're responsible for the raw design tokens, we architect our design tokens to conform to the parser capabilities. If the parser logic is updated, we update our tokens, as needed. We use our own parser to generate various micro libraries that define platform-specific assets, which are then consumed by downstream projects to build out various UIs. Our internal stakeholders rarely (if ever) need to know the exact relationship between the tokens and parser.

With that aside...

Theoretically, any token value could be an array such that the first value would be the fallback default if a specific platform doesn't support an array of values. However, shadows are definitely one token type that I agree would benefit from being an array by default, because I can't think of a recent project I've worked on that only used a single shadow value.

ilikescience commented 9 months ago

Wanted to see if I could come up with a formal proposal to incorporate some of the feedback here! I really want to get multiple shadows into the spec, since it's so common in my own use cases (and others', as shown in this thread).

So, a shadow looks like this:

{
  offsetX: dimension
  offsetY: dimension
  blur: dimension
  color: color
  spread: dimension     // optional
  inset: boolean        // optional
}

Technically you can define a css drop shadow without a color, which essentially causes its color to have the value inherit. I think it's reasonable to require color, and the spec does so today.

Spread is no longer required. I believe it's possible to define shadows in CSS (box, drop, and text shadows included) and Flutter without spread, and in Swift and Kotlin I don't see any use for it.

As for how parsers know the difference between drop, box, and text shadows, I think the simplest first step would be to let the parser be "dumb" and not try to figure out which is which, but to put it on the user to define the token correctly for their use case, ie don't add spread and inset if you intend to use the parsed css value in a filter: drop-shadow() context. A parser might even allow a user to specify that they want a particular token formatted to be compatible with CSS drop-shadow or text-shadow; either way, I don't think we should try to write inference logic into the spec.

As for multiple shadows, we allow the $value of a shadow token to be a single shadow or an array of shadows. This is pretty forgiving to the author at the expense of an little bit of parser complexity. I don't think it'll break the bank, though.

So, the final token looks like:

{
  "shadow": {
    "small": {
      "$type": "shadow",
      "$value": [
        {
          "color": "rgba(0, 0, 0, 0.12)",
          "offsetX": "0px",
          "offsetY": "1px",
          "blur": "1px",
          "spread": "1px",
          "inset": true
        },
        {
          "color": "rgba(14, 14, 17, 0.40)",
          "offsetX": "0px",
          "offsetY": "2px",
          "blur": "5px",
          "spread": "0px"
        }
      ]
    }
  }
}

wdyt? any objections to moving this forward to the editors for consideration?

acstll commented 8 months ago

any objections to moving this forward to the editors for consideration?

to remove that little bit of parser complexity you could make $value take an array only; for single-value shadows, the array has only 1 shadow…

In any case, I think this is perfect to move it forward.

danosek commented 3 months ago

Looks great. The only missing piece of a puzzle is a definition of the color in multiple spaces discussed in #137. But this is a issue that can be resolved later, I think.

drwpow commented 1 month ago

I’ve just put up a proposal ✅ accepting this original proposal here: #246.

This is NOT an official decision yet! This is only a way to source feedback, and push this forward. I tried to distill the thoughts & opinions expressed in this thread into a formal spec change. What I saw in this thread was generally unanimous agreement about shadow changes, which I believe I’ve implemented without changes. But please let me know if I missed a suggestion! Any/all feedback is welcome.

drwpow commented 1 month ago

Just popping in to say with #246 accepted and merged, all proposals have been accepted for the next version of the DTCG format (If your suggestion was accidentally missed, let us know! 🙏 ).

kaelig commented 1 month ago

Thank you @drwpow I appreciate this may be a bit fast, but I merged the changes proposed above because there seems to be consensus around it, and several implementations already support it (SD and Cobalt).

We haven't cut a new snapshot of the spec yet (this will make it into Editors Draft 3).

Two more changes are coming that will impact this part of the spec: