feat(Label): add color prop

[bmdalex]

feat(label): color prop

Description

This PR:

• adds color prop to Label component
• implements basic color scheme for Label colors
• creates Label color examples
• closes [RFC] Color prop for components where it can target multiple areas (content, background) #636

Latest updates on the API (@mnajdova)

Final implementation details. The color prop in the label can receive an object containing all parts of the color scheme: foreground, background, border, hover, shadow (we may want to trim this down to different objects in each component, or if we have component that can have multiple colors inside, we can always have the scheme).

How it works

1. The user specifies one color and all colors are calculated from the scheme

<Label color='red' content='Red' />

The color can be one of the following: 'primary' | 'secondary' | 'blue' | 'green' | 'grey' | 'orange' | 'pink' | 'purple' | 'teal' | 'red' | 'yellow' | string

2. The user can specify object as the value for the color, and those colors depending on the part where they are defined (foreground, background) are picked up from the color scheme

 <Label color={{foreground: 'red', background: 'green'}} content='Red on orange' />

3. The user can specify again object as color, and any of the color can be a custom color, or if is not found in the color scheme, will be returned as the actual HTML color.

<Label color={{foreground: 'red', background: 'blue'}} content='Red on blue' />

As the blue does not exists on the color scheme, the HTML blue color will be returned. The same will be applied if the color provided is in form of a hex value.

How is the color scheme in the variables defined

The color scheme has defaultValues for all parts (foreground, background border etc), but the component defining can override this (for example the Label can define some shades of grey as defaults for the foreground and background). In order for this to work, there is an util method added called getColorSchemeWithCustomDefaults. Here is how the color scheme is defined inside the labelVariables:

 colorScheme: getColorSchemeWithCustomDefaults(
      {
        foreground: 'rgba(0, 0, 0, 0.6)',  // default label foreground
        background: 'rgb(232, 232, 232)', // default label background
      },
      siteVars.colorScheme,
    ),

Here is how all colors available in teams theme looks like:

[bmdalex]

@layershifter fyi

[kuzhelov]

speaking on the ask for inversion mechanism - we might start with a simple concept of on colors where 'on' color (for text/icons/anything that could be placed on top of background) will be provided in two variants: for light background (then black color may be used, as an example) and dark ones (for instance, then white color may be used for text). This will require the following declaration for on color from the client

onColor = {
  light: #333, // will be used on dark background
  dark: #333 // will be used on light background
}

Then we might apply logic that will precompute appropriate 'on' color (dark or light one) for each color used in the user's theme.

Note that if client will need to customize the 'on' colors generated, it might still be possible:

// pseudo-implementation, just to provide an idea
onColor: {
    
  light: #333, // will be used on dark background by default
  dark: #333 // will be used on light background by default
  
  // OVERRIDES
  backgrounds: {
     primary:  <some custom color>, // will be applied for all 'primary' tints (100 - 900)

     // with these variants being used where appropriate for different tints of secondary
     secondary: {
         light: ... 
         dark: ...
     },
     
     // absolute fine-tuning
     pink: {
         ....
         '600': <color for exact tint of pink>
     }  
  }
}

[bmdalex]

@kuzhelov can you please add your proposal to #636 ?
there's a big discussion on this happening over there

[bmdalex]

single -> simple might be better in conjunction with complex what do you think?

[mnajdova]

Agreed, it should either be single | multiple or simple | complex (I like more the second combination)

[bmdalex]

great work Marija 👍 please take a look at my comments before we merge

[layershifter]

Should point to Unreleased

[mnajdova]

Thanks, moved

[layershifter]

I am not sure about this idea, can we omit it for initial implementation?

colorScheme: {
  ...siteVars.colorScheme,
  default: {
    ...siteVars.colorScheme,
    foreground: color,
    background: 'rgb(232, 232, 232)',
  }
}

[mnajdova]

Not sure then how will the components define their default scheme, as in the label we have grey foreground and background, but for the button is white background and grey foreground...

[layershifter]

1. We don't need destruction there

_.get(colorScheme, colorProp as string, colorScheme.default)

2. If we will revert the condition, we will be able to omit as string

type of colorProp === 'string'
    ? _.get(colorScheme, colorProp, colorScheme.default)
    : { ...colorScheme.default, ...getColorSchemeFromObject(colorScheme, colorProp) }

[layershifter]

This idea looks quite confusing to me :(

I have a definition for blue in the color scheme, but blue is a valid CSS color, https://www.quackit.com/css/css_color_codes.cfm

[mnajdova]

Totally understand your point, but I don't have better proposal at this moment. We have to allow the user somehow to define the colors by name (the ones we have in the color palette, but still if we don't have them, we would end up with the CSS color.. If they define it as hex value then it is clear, but in the other case, is kind a confusing. Still, we have to use color names for the color prop, so I don't have any other proposal at this moment)

[layershifter]

// uses `red` from schema
<Label color='red' />

// uses CSS `red`, i.e. #FF0000
<Label color={{ background: 'red' }} />

// still uses CSS colors, but you can get them from schema
<Label color={(colorSchema) => ({ background: colorSchema.red.background })} />

What about color as function?

[mnajdova]

Couple of thoughts on this:

• not sure what would be the use cases where the user will want to use the CSS colors, when they have defined schema for which colors will use
• more-over why should the prop in our API as default returns just some CSS color - as a user would make me wonder why we have special API for that

On the other hand, I understand the confusion, but would rather not over-complicate the API now, and wait for complains if there will be any in the future.

[bmdalex]

totally support @mnajdova 's points

if the color scheme name collides with the CSS color name it's by design and it's expected since the developer should stick to a theme color as much as possible

if the developer reaaaally wants the actual CSS red (which is not recommended since they're supposed to work within the theme constraints), they can still use the hexcode (#FF0000)

[levithomason]

Clarification, the color prop exists specifically to restrict the developer from deviating from the theme color. The color prop is by design restrictive and imprecise. It should not allow defining where or how the color is applied nor what specific color it is, just the name.

Where and how the color is applied is the job of the designer through the theme. All props should be high-level, natural language, imprecise, and void of implementation.

Think of the color prop as used in this statement "Click on the red label." Now, imagine you ask 10 designers to draw the label. There are many ways to do this, which is exactly what we want. We want flexibility through imprecise natural language.

[kuzhelov]

just thoughts aloud: would rather prefer them being aligned horizontally, as it is commonly done by other libraries for tag components - but yes, there is no good layout component for that yet.

[mnajdova]

Waiting for the Flex component then :)

[levithomason]

Always delete as much code as possible:

1. No Grid is necessary, especially since it is a very complex solution to a very simple problem (add a space).
2. Lodash is not necessary to Start Case the color names, just show the prop values (more helpful as well).
3. Only use top level parent components, not the directly exported children (Provider.Consumer vs ProviderConsumer)

import * as React from 'react'
-import * as _ from 'lodash'
-import { Label, ProviderConsumer, Grid } from '@stardust-ui/react'
+import { Label, Provider } from '@stardust-ui/react'

const LabelExampleColor = () => (
-  <Grid
-    columns="auto"
-    styles={{ justifyContent: 'left', justifyItems: 'left' }}
-    variables={{ gridGap: '10px' }}
-  >
    <ProviderConsumer
      render={({ siteVariables: { colorScheme } }) =>
-        _.keys(colorScheme).map(color => (
-          <Label key={color} color={color} content={_.startCase(color)} />
+       Object.keys(colorScheme).map(color => (
+          <span key={color}>
+            <Label color={color} content={color} />{' '}
+          </span>
        ))
      }
    />
-  </Grid>
)

[kuzhelov]

as we've discussed, hover is a questionable member here, as it rather represents a state and not a color per se. At the same time, there is no need to introduce this complication for now - as there are several options of how this issue could be addressed

[mnajdova]

Agreed, will remove it for now.

[kuzhelov]

it would be great if we will be able to reduce color members siteVariables directly expose - currently almost each new color-related concept results in the additional prop being added, but what is more important - those props are all cohesively connected with each other, so we might want eventually to introduce a dedicated object for them

[bmdalex]

not sure I follow your point @kuzhelov can you please expand or offer an example?

[mnajdova]

@kuzhelov are you suggesting having maybe one colors object that will contain the scheme, colors and everything related to the coloring mechanism? I would really avoid that refactoring as part of this part, but I can create an issue and open PR after this one gets in.

[kuzhelov]

@Bugaa92, sure - currently we have the following set of props being exposed by siteVariables: colors, contextualColors, emphasisColors, colorScheme, naturalColors, etc - there are just plenty of them, and all are aimed to describe color concept. Here I am proposing to introduce single color prop that will be an object with all these being assembled as props:

siteVariables = {
   color: { 
      scheme, 
      contextual,
      natural,
       ...
   }
   ...
}

This will aid maintainability (as all closely related aspects will be semantically grouped), as well as will not pollute the siteVariables object itself. More than that, on the client perspective it will be much cleaner to see the entire set of objects from which colors could be consumed from, as everything related to color will be assembled under single object.

[bmdalex]

@kuzhelov I like it, but would agree with @mnajdova to postpone this for a follow-up PR and create an issue for it

[kuzhelov]

see this being a very brittle thing that is tricky to maintain. I would be great to inline this isLight flag value to the color variant itself, so that in case if collection of colors will change (or even if color name will change), we won't bother about

• adding new color to the list / remove color from the list
• change the name of the color accordingly

We definitely should think on some mechanism that will introduce this ability to select foreground color depending on the background.

[bmdalex]

@kuzhelov agreed, what is your proposal?

add background as a key to the ColorVariants type?

[kuzhelov]

yes, essentially I would propose to reconsider the set of props of the ColorVariants type - and just add the only necessary to inline this information currently kept by lightBackgroundColors array. It may be a background prop, or maybe other set of props (like light and dark, and this will allow us to properly apply colors on top of each other).

There are several corner cases that we won't be able to cover with the naive approach, thus I would rather like to open a separate discussion around exact implementation - but generally, yes, would rather see this information as being part of the ColorVariants type.

[bmdalex]

@kuzhelov got it, agreed it'd be a better way to do things and was thinking of something similar myself
@layershifter can you weigh in as well?

[mnajdova]

Will keep a track of this in an issue together will the other proposals for improvements regarding the color mechanisms.

[kuzhelov]

same story here - if we will be able to agree on some way to define the fact that specific color is a 'light' one, we would solve a maintainability issue and won't need to duplicate common related functionality (like isLightBackground)

[bmdalex]

see my question above:
https://github.com/stardust-ui/react/pull/647/files#r247172459

[kuzhelov]

in future see this being reduced to a single concept of 'merging color schemes' - this should simplify the scheme object itself, make merging logic simpler and remove default prop

[kuzhelov]

is it, essentially, mergeColorSchemes?

[kuzhelov]

also, this definitely shouldn't be a user responsibility to bother about this stuff. I would expect that colors object will be just directly consumed from the style function, like that (one of the options):

root: ({ props, variables, colors }) => ({
   foreground: colors.foreground,
   ...
})

[bmdalex]

@kuzhelov
don't forget about the p.color parameter coming from the user; generateColorScheme takes that argument and returns the ColorScheme object for p.color; maybe the name is confusing and should be:

generateColorSchemeForColorProp or something more concise

[mnajdova]

I've added this with the latest commit (those are merged in renderComponent). Please take a look.

[levithomason]

The commonPropTypes abstraction is growing out of hand. We already have customPropTypes. Also, the function calls are unnecessary.

Always try to delete as much code and work as possible. Consider this line:

...commonPropTypes.createCommon({ color: 'complex' }),

1. commonPropTypes duplicates the job of customPropTypes
2. Requires a function call
3. Requires constructing an options object (of the exact shape that the returned object will contain)
4. Causes indirection, obscures implementation, and requires spreading

We can achieve this much more simply like this:

color: customPropTypes.complexColor

1. No need to create or import commonPropTypes vs customPropTypes
2. No need to call a function (createCommon())
3. No need to create an options object
4. Is obvious as the entire implementation is explicit and simple to read