Skip to content
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

CSS background-color, CSS color, and layer recipes in the DesignSystemProvider #3213

Closed
nicholasrice opened this issue Jun 1, 2020 · 9 comments
Assignees
Labels
area:fast-foundation Pertains to fast-foundation closed:done Work is finished improvement A non-feature-adding improvement

Comments

@nicholasrice
Copy link
Contributor

nicholasrice commented Jun 1, 2020

Problem:

When using the DesignSystemProvider there exist several disconnects that make the implementation difficult to use:

  1. The provider does not apply a CSS background-color to the element aligning to the designSystem.backgroundColor property
  2. The provider does not apply a CSS color to the element aligning to the designSystem.backgroundColor property
  3. The provider's backgroundColor property cannot be assigned a recipe.

1 and 2 make the out-of-box experience bad because the color of non-FAST content will be inaccessible and obviously incorrect.

3 is more of an interop problem within the FAST system. The FAST system provides and encourages use of the "layer" recipes to be used as app-region background colors but this implementation of the DesignSystemProvider does not facilitate doing so.

Proposal

1. and 2. - CSS color & background-color

1 and 2 can be remedied rather simply. When the backgroundColor property of the provider is explicitly set, the provider should attach the following stylesheet:

css`
:host {
    background-color: var(--background-color);
    color: var(--neutral-foreground-rest);
}
`.attachBehavior(
	neutralForegroundRestBehavior
);

While generally desired, I can imagine cases where a user may not want this behavior so we would want a mechanism to disable attachment. This could be done with a boolean attribute:

<fast-design-system-provider draw-background-disabled background-color="#FFFFFF">
</fast-design-system-provider>

or with a property:

FASTDesignSystemProvider.drawBackgroundDisabled = true;

3. - Recipe backgrounds

Another commonly desired cases is to set the value of the DesignSystem.backgroundColor to the product of a recipe. This often crops up in cases where the layer recipes are used - neutralLayerL1, neutralLayerL2 , etc.

We could enable the following:

FASTDesignSystemProvider.backgroundColor = neutralLayerL1;

When assigned a recipe / fn, the provider would evaluate the recipe when setting FASTDesignSystemProvider.designSystem.backgroundColor and which would in turn assign the --background-color CSS Custom Property to the product of the recipe.

A question arises from the above of which designSystem object the above resolution function should be generated with. There are three viable options:

  1. The parent provider's design system
    1. The downside to this approach is that a user could not set the baseLayerLuminosity and the background color on one provider to achieve a light/dark mode change. In fact, the recipe would not see any designSystem changes on the element with the background change. I think this is a big enough miss that this option should not be selected but it is included for completeness.
  2. The local design system
    1. This option would invoke the recipe to resolve the background color with the local design system object. This facilitates cases like light/dark mode switching using a single node, because the recipe would see the designSystem changes applied to the node.
    2. The only reasonable down-side or disconnect I see to this is that it could create recursive behavior that would likely be undesirable. Because this recipe sets a property on the designSystem (designSystem.backgroundColor), if the recipe uses designSystem.backgroundColor in determining the color product then each invocation will be relying on the previous invocations product - recursive color resolution. This would likely be an unintuitive and undesired behavior.
  3. The hybrid design system
    1. To address the issue described by 2.2, we could supply the fn a hybrid design system, where every property besides the property being set is derived from the local designSystem object, and the property being set (backgroundColor) is derived from the parent design system.
    2. Another solution would be to omit the color from the invocation of the recipe so that it cannot be relied upon.
@triage-new-issues triage-new-issues bot added the status:triage New Issue - needs triage label Jun 1, 2020
@nicholasrice nicholasrice added the improvement A non-feature-adding improvement label Jun 1, 2020
@triage-new-issues triage-new-issues bot removed the status:triage New Issue - needs triage label Jun 1, 2020
@nicholasrice nicholasrice changed the title Add a "background" component CSS background-color, CSS color, and layer recipes in the DesignSystemProvider Jun 4, 2020
@eljefe223
Copy link
Contributor

I'm slight confused on 3.1 what do you mean by "besides the property being set"? Also I think would expect the background color to be derived from the parent. Isn't that how our background component works in react?

@nicholasrice
Copy link
Contributor Author

That just means that every property except background color would come from the local design system, and the background color would come from the parent design system:

// psuedo-code
this.designSystem.backgroundColor = this.backgroundRecipe(
  {
    ...this.designSystem,
    backgroundColor: this.context.designSystem.backgroundColor
  }
)

Yea the React Background component essentially implements 3.1 and suffers from the described issue. Maybe thats okay/desired but wanted to explore other ideas.

@bheston
Copy link
Collaborator

bheston commented Jun 4, 2020

This is something we've needed for a while, so I'm glad to see this going through. I'm trying to think through the ways this would apply to other properties as well.

Option 1 does sound somewhat limiting for this situation, but it has the benefit of being very deterministic, essentially overriding settings at each level in sequence.

I think option 2 will be too limited because of the cascading nature of many of the properties, particularly color. Thinking of background color alone, it seems reasonable that a non-layer-based design system would derive a background from the container. Darker or lighter or something. This has also come up in density scenarios where a compound child component wanted to be more or less dense than its parent, and the only way to do that was to wrap a consumer and a provider together to determine the final value. In our new (target) model I think this shifts from density to base type size, but I could see the same requirement if you wanted to make a nested section smaller or larger, but still relative.

I think a hybrid approach sounds the easiest to comprehend, but I wonder if there's a way to make the idea of super vs this more explicit. I can't determine if it's always the same property (the one being set) that would need to come from the parent design system though. For background this seems true. For font size perhaps as well.

We don't know where a function is being used though, right? Like if someone has a function for adjusting type size that it would reference the parent type size property, but all other values would come from the local properties.

I think 3.1 sounds best, though it has caused some confusion in the existing Background implementation. Does the idea around super / this sound useful in any way?

@EisenbergEffect
Copy link
Contributor

@nicholasrice Can we close this as implemented?

@EisenbergEffect
Copy link
Contributor

@nicholasrice This can be closed now yes?

@nicholasrice
Copy link
Contributor Author

nicholasrice commented Jul 21, 2020

@EisenbergEffect 3.) isn't addressed yet, so lets keep this open for the time being.

@EisenbergEffect EisenbergEffect added the status:in-progress Work is in progress label Jul 21, 2020
@EisenbergEffect EisenbergEffect added this to In Progress in Architecture Jul 22, 2020
@EisenbergEffect EisenbergEffect added this to In Progress in Design Systems Jul 22, 2020
@nicholasrice nicholasrice moved this from In Progress to Backlog in Design Systems Jul 29, 2020
@EisenbergEffect EisenbergEffect moved this from Backlog to In Progress in Design Systems Aug 3, 2020
@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the warning:stale No recent activity within a reasonable amount of time label Dec 25, 2020
@EisenbergEffect
Copy link
Contributor

@nicholasrice Shall we close this one as "done" under the new system?

@stale stale bot removed the warning:stale No recent activity within a reasonable amount of time label May 11, 2021
@nicholasrice
Copy link
Contributor Author

Yep! Woot!

Design Systems automation moved this from In Progress to Done May 11, 2021
Architecture automation moved this from In Progress to Done May 11, 2021
@EisenbergEffect EisenbergEffect added the closed:done Work is finished label May 11, 2021
@EisenbergEffect EisenbergEffect removed the status:in-progress Work is in progress label May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:fast-foundation Pertains to fast-foundation closed:done Work is finished improvement A non-feature-adding improvement
Projects
Architecture
  
Done
Development

No branches or pull requests

4 participants