WIP color system refactor #4614
Conversation
| const referenceIndex = this.closestIndexOf(reference); | ||
| let startIndex: number; | ||
| let source: ReadonlyArray<SwatchRGB>; | ||
| const direction = directionByMode(reference); |
There was a problem hiding this comment.
This represents a change from the current functionality, but I think we should leave the decision about the direction to the individual use case / recipe. The notion of light or dark mode is for the UI as a whole, and it's harder to deal with on an individual color. We at least need a preferred direction or some way to look for consistency across an experience. A real example where this comes up is wanting to keep white text over various accent colors. If the background supports it, even if black would have more contrast, if white passes it should be used.
Thinking of this more from an lower-level class perspective, if I'm asking for a lower contrast value like 2:1 I'm probably using it in some way where I am less concerned with light / dark mode and would want to specify the direction.
There was a problem hiding this comment.
I might be missing context, but I do have an idea of what @bheston is getting at with this:
A real example where this comes up is wanting to keep white text over various accent colors. If the background supports it, even if black would have more contrast, if white passes it should be used
In the above scenario, is it too much to ask them to provide an alternative recipe? Do we need a binary switch here? If you want to change how a color is resolved, I think the system now provides a method for that with DI. But I'm not sure we need to expose that. That type of configuration as a part of the core color library becomes expensive and something that needs to be resolved every time. Is there a reason that wholesale replacing the recipe in the case above is not reasonable? Full disclosure, I think that's currently the expectation from the team asking me for the above behavior. I don't know that it's universal enough to inform the recipes themselves or to take the potential perf hit of checking every time.
There was a problem hiding this comment.
We've had feedback in Fluent UI to update the recipe for our accent buttons to defaulting to white rather than black when the above is true, but I don't think it's necessary in each recipe personally.
| restDelta: number, | ||
| hoverDelta: number, | ||
| activeDelta: number, | ||
| focusDelta: number, |
There was a problem hiding this comment.
Let's not carry the focus and selected colors forward in this update.
Selected was planned to be deprecated long ago because a component in a 'selected' mode should also respond to hover and active states, and therefore needs its own recipe. That's where neutralFillToggle came in.
And for focus all of our current offsets are exactly the same as rest, we haven't applied them in styles, and it shares the same issue with 'selected' components. A focused component can also be hovered or pressed.
| * @param color - The color to check to mode of | ||
| * @returns boolean | ||
| */ | ||
| export function isDarkMode(color: Swatch): boolean { |
There was a problem hiding this comment.
I touched on this in Palette above, but I feel like the name of this function doesn't encourage correct usage. It's a concise name though. More accurately it's something like isCloserToBlack. Horrible name, but I'm open to the idea of removing this from the notion of light or dark mode UI.
nicholasrice
changed the base branch from
master
to
features/design-system-vNext
nicholasrice
changed the base branch from
features/design-system-vNext
to
master
|
Closing in favor of #4623 |
Pull Request
📖 Description
Draft PR for color system updates to work better with Design Token work.
🎫 Issues
👩💻 Reviewer Notes
📑 Test Plan
✅ Checklist
General
$ yarn changeComponent-specific
⏭ Next Steps