[SUGGESTION NEEDED] feat(Icon): introduce 'background' property

[kuzhelov]

Icon ('inverted' variant)

This property will introduce possibility to specify Icon's background color. In contrast with just inverted property being introduced, with background property we will provide all the necessary means to customize look of the icon to the client - without necessity to use heuristics for detecting which foreground color would be more appropriate for the given background one.

TODO

• Conformance test
• Minimal doc site example
• Stardust base theme
• Teams Light theme
• Teams Dark theme
• Teams Contrast theme
• Confirm RTL usage
• W3 accessibility check
• Stardust accessibility check
• Update glossary props table
• Update the CHANGELOG.md

API Proposal

background: string

<Icon name='...' background='teal' color='white' />

[codecov]

Codecov Report

Merging #86 into master will decrease coverage by 0.04%.
The diff coverage is 63.63%.

@@            Coverage Diff            @@
##           master     #86      +/-   ##
=========================================
- Coverage   69.94%   69.9%   -0.05%     
=========================================
  Files          74      74              
  Lines        1198    1203       +5     
  Branches      227     209      -18     
=========================================
+ Hits          838     841       +3     
- Misses        355     357       +2     
  Partials        5       5

Impacted Files	Coverage Δ
src/components/Icon/iconVariables.ts	100% <ø> (ø)	⬆️
src/components/Icon/Icon.tsx	100% <100%> (ø)	⬆️
src/components/Icon/iconRules.ts	84.61% <60%> (-5.87%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ac3ded...2089e9f. Read the comment docs.

[mnajdova]

I have one concern about this. Is the white always the value for the color? Should we have some function for returning (white, grey, black) variant that works best with the background? Something like this: http://jsfiddle.net/PXJ2C/ And do we want to add a way to the user to also change the color of the text?

[kuzhelov]

actually, this is quite close to the point that you've expressed in another PR: #85. While I would rather like to avoid any heuristics to be introduced here, I totally support the idea of introducing additional means for customization.

Actually, if we would have background property, we won't need the inverted one. Let me see what would be a minimal set of changes that will lead us to common denominator with #85 as well.

[mnajdova]

I totally agree, having the background and color properties should be enough for customizing the icon. And I agree with your point for maybe introducing borderColor prop, but having it to default to the background or the color props.

[levithomason]

This is a public facing change. Please update the CHANGELOG.md.

There should be an entry for this PR added under Features in UNRELEASED. See the instructions there.

requested changes are provided

[levithomason]

I'm hesitant on this one. It is adding a CSS properties directly to the public API. We are targeting an API that is a high level abstraction and implementation agnostic. It also sets precedent where users will expect other CSS properties to be exposed there: fontSize? fontWeight? etc.

Sticking to descriptive and implementation agnostic APIs means a theme can choose what the concept looks like. As an example, what if inverted in my theme doesn't have a background color at all? What if icons don't have any backgrounds in a theme? Then, this prop will not make sense.

Counter Proposal

API vs Styles

API - design language / component concepts
Style - CSS implementation

The prop color doesn't say where or how this color will be used, just that it is being applied in someway. The prop background with a color value dictates the exact implementation. Therefore, it is defining a style, not a component concept.

If a user wants to set the background color of a component, I propose it should be done with the styles prop:

<Icon styles={{ root: { background: 'teal' } }} />

If they need to access a site variable to use a special color:

<Icon styles={{ root: siteVars => ({ background: siteVars.specialColor }) }} />

Theming

Because background defines explicit style values, I can't see a way of this working in light/dark/contrast. By definition, the theme wants to set the background color, however, setting a prop to this value would not give the style function any room to use a different value. It could simply ignore the user's input and override it, but this would be confusing and not ideal for consumers.

[kuzhelov]

sorry, @levithomason, cannot agree with the fact that we are introducing 'CSS Property' to the interface. If you take a look on the implementation, this property is not equivalent to just adding the CSS property - it does more. For example, it applies padded style to the icon. So, for that reason, one thing that I am absolutely against is to introduce it in styles as CSS property - it is higher level aspect.

The next question is about whether it should be introduced to variables or props. As you might notice, this is introduced in props and is duplicated in props (with props taking precedence). The reason of having it in props is that it will complement color property that is already there (and, by the way, it also while corresponding to CSS name, does much more that just setting foreground color) - otherwise the set of properties won't be consistent to achieve quite common case of circular button with specific background - the one that is used for presence indicator.

So, overall, I think that

• asking clients to introduce background as a rule is not a solution - here we are dealing with an aspect that is higher than CSS property by its semantics
• introducing it to props makes sense, as with that we will have it being consistent with already introduced color prop

Few comments in addition:

The prop color doesn't say where or how this color will be used, just that it is being applied in someway. The prop background with a color value dictates the exact implementation.

Cannot agree here. Those both have clear correspondence to CSS properties, and, thus, I am not sure why we are saying that color doesn't reveal anything, while background reveals implementation details. Those both are higher level of abstraction over CSS props.

Because background defines explicit style values, I can't see a way of this working in light/dark/contrast

There are following ways to do that. Suppose that we have the following cases:

• component is used in isolation by the client. Then it is enough to introduce corresponding variable's value in theme:

const themeComponentVars = {
    Icon: {
         background: ...
    ...
    }
}

• if component is used as a building block for other component, background variable could be passed tied to one of the higher-order component variables

[levithomason]

TL;DR

We can merge this for progress. I'm not sure the concept will hold up, but I don't have a better proposal or path for this work right now.

Would like to share my thoughts as well...

---

I think I understand your points on background now. Both the color and background props share CSS properties of the same name, however, both props are trying to name a concept implemented by a component.

In the context of CSS, color sets text color while background sets the background color, however, in the context of a component these are concepts. A component can apply a color however it chooses. I can also see how a component could can have a background, more as a shape or additional element opposed to an explicit CSS value only.

I'd like to give more info on the relationship of color and inversion.

Color

Here is a Segment that contains a Button, Menu, and Icon. Each component is shown with and without color. Notice how each blue colored component applies color in its own way:

Inverted

The concept of "inverted" is creating contrast. Here are all the same components, but adding the inverted prop to all of them. Notice how "inverted" doesn't necessarily mean "use a background".

Also, inverting a component does not make any of its other variants invalid. The icon can still use color, circular, and bordered variants. The button can still appear default or basic (reduced complexity) while being inverted.

Background

I would like to see more components and real world usages that use this concept of "add a background" before we add it as a concept to the library. I'm not sure that the idea of backgrounds is a component concept that scales across the library.

I also want to be very cautious about adding any new concepts while we're in the infancy of our development. I want to introduce a staging mechanism where concepts like "background" can be introduced and used, but without automatically becoming first class concepts and APIs. Something like an opt-in so we can better test things and have slower more confident roll outs.

Conclusion

All said, we don't have a staging system yet for new ideas to thrive. I also have doubts about whether or not inverted will survive as a concept once we have dark/contrast themes since those ideas seem to overlap. I wanted to share my thoughts and background to keep us in sync with these idea. Let's merge with the understanding that this concept of background might change in the near future as we introduce feature staging, theme switching, and more components.

[kuzhelov]

Just to put final thoughts on this. We have agreed that either of these two options would work for us - as well as will be consistent for the client:

• provide color and background both as props and variables
• provide color and background as variables only

---

I would suggest the second one, and here is the reason why. If we'll consider the first option, we would have two ways of doing the same thing - and while it is not a cons per se, the problem could arise in the following context.

Suppose that someone is working on higher-order component (composite one), and this work has resulted in the following component tree to be rendered for this:

<ElementType>
    <Icon color='red' />
</ElementType>

There is a quite serious problem with this approach: now there are no ways to customize visual look for Icon component, as prop-provided values will take precedence over variables! This will be quite astonishing surprise for the client, to say the least.

In contrast, an option where these two will be introduced as variables only will open the door for theming (and honoring siteVariables values), as now the following would be possible, with all scenarios of the alternative option being covered as well.

<ElementType>
    <Icon variables={ siteVariables => { color: siteVariables.brand } } />
</ElementType>

---

Also, if merged component's variables will be passed to the renderComponent function (see discussion #121), it will be possible to do the following as well:

...
// note that 'variables' are merged ones!
renderComponent({ .., variables } => (
  <ElementType>
      <Icon variables={ color: variables.color } } />   // inherits foreground color of the parent component
  </ElementType>
)

With that we will be able to significantly reduce amount of siteVariables necessary to handle these cases.

@levithomason, please, share your thoughts on that.

[levithomason]

Let's prevent developers from passing props directly to CSS colors by instead using keys from a color pallet in the style function:

// iconRules.ts
  ...
  color: variables.textPallet[iconColor],
  ...

[kuzhelov]

closing as now corresponding PR is introduced to the new repo: microsoft/fluent-ui-react#47