-
Notifications
You must be signed in to change notification settings - Fork 64
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
Shadow type feedback #100
Comments
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) |
Totally agree with @drwpow that supporting arrays of shadow objects is critical for my use cases. Is the idea that |
I don't see an |
Yes, that appears to be an oversight. Seems like an |
@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' 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):
|
Akin to #100 (comment) and #100 (comment), a boolean Figma provides two ways to define shadows (Drop Shadow and Inner Shadow): Inner Shadow corresponds to the use of the 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
|
How would a CSS translation tool know if a 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
|
This might be connected to #134 |
Another thought: Should the standardization of design tokens empower (to build custom design systems) or limit down (to follow a given approach)? Is there something in between? Like a name-spaced type set? {
"dropShadow": {
"$type": "@web/shadow",
"$value": {
"offsetX": "0px",
"offsetY": "0px",
"blur": "2px",
"spread": "4px",
"color": "#cccccc"
}
}
} or even an offer of multiple types? {
"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. |
My point was to show that the concept of a |
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. |
In addition to all other already mentioned shadow types should we also consider CSS filters |
It seems like the spec as-is would already be perfect for |
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 |
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 |
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 {
"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);
} |
Agreed with @lukasoppermann |
Also agree with @lukasoppermann 👍
|
Just been catching up on all this great feedback. Thanks y'all! First off, I completely agree that As for the 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 ( 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? 😅 |
Shadow array
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 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
Not sure about this. The one thing that bothers me is that the inset value changes the meaning of the x and why values. |
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.) |
About
I don't think that even if |
Another question about multiple shadows within one. CSS SwiftUI In Figma you can create styles with multiple effects. |
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:
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:
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. |
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:
Technically you can define a css drop shadow without a color, which essentially causes its color to have the value 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 As for multiple shadows, we allow the 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? |
to remove that little bit of parser complexity you could make In any case, I think this is perfect to move it forward. |
Looks great. The only missing piece of a puzzle is a definition of the |
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.
|
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! 🙏 ). |
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:
|
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.
The text was updated successfully, but these errors were encountered: