Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

feat(Label): add color prop #647

Merged
merged 23 commits into from
Jan 11, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

### Features
- Add `Loader` component @layershifter ([#685](https://github.com/stardust-ui/react/pull/685))
- Add `color` prop to `Label` component @Bugaa92 ([#647](https://github.com/stardust-ui/react/pull/647))

### Fixes
- Fix focus outline visible only during keyboard navigation @kolaps33 ([#689] https://github.com/stardust-ui/react/pull/689)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import * as React from 'react'
import * as _ from 'lodash'
import { Label, ProviderConsumer, Grid } from '@stardust-ui/react'

const LabelExampleColor = () => (
<Grid
Copy link
Contributor

@kuzhelov kuzhelov Jan 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for the Flex component then :)

Copy link
Member

@levithomason levithomason Jan 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

image

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>
)

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)} />
))
}
/>
</Grid>
)

export default LabelExampleColor
7 changes: 6 additions & 1 deletion docs/src/examples/components/Label/Variations/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,14 @@ const Variations = () => (
<ExampleSection title="Variations">
<ComponentExample
title="Circular"
description="A label can be circular."
description="A Label can be circular."
examplePath="components/Label/Variations/LabelExampleCircular"
/>
<ComponentExample
title="Color"
description="A Label can have different colors."
examplePath="components/Label/Variations/LabelExampleColor"
/>
</ExampleSection>
)

Expand Down
7 changes: 5 additions & 2 deletions src/components/Label/Label.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,21 @@ import {
ChildrenComponentProps,
ContentComponentProps,
commonPropTypes,
ColorComponentProps,
} from '../../lib'

import Icon from '../Icon/Icon'
import Image from '../Image/Image'
import Layout from '../Layout/Layout'
import { Accessibility } from '../../lib/accessibility/types'
import { ReactProps, ShorthandValue } from '../../../types/utils'
import { ComplexColorPropType } from '../../lib/commonPropInterfaces'

export interface LabelProps
extends UIComponentProps,
ChildrenComponentProps,
ContentComponentProps {
ContentComponentProps,
ColorComponentProps<ComplexColorPropType> {
accessibility?: Accessibility

/** A Label can be circular. */
Expand Down Expand Up @@ -55,7 +58,7 @@ class Label extends UIComponent<ReactProps<LabelProps>, any> {
static className = 'ui-label'

static propTypes = {
...commonPropTypes.createCommon(),
...commonPropTypes.createCommon({ color: 'complex' }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

circular: PropTypes.bool,
icon: customPropTypes.itemShorthand,
iconPosition: PropTypes.oneOf(['start', 'end']),
Expand Down
67 changes: 66 additions & 1 deletion src/lib/colorUtils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import * as _ from 'lodash'
import { SiteVariablesInput, ColorVariants, ColorValues } from '../themes/types'
import {
SiteVariablesInput,
ColorVariants,
ColorValues,
ColorSchemeMapping,
ColorScheme,
} from '../themes/types'
import { Partial } from '../../types/utils'
import { ComplexColorPropType } from './commonPropInterfaces'

export const mapColorsToScheme = <T>(
siteVars: SiteVariablesInput,
Expand All @@ -9,3 +17,60 @@ export const mapColorsToScheme = <T>(
{ ...siteVars.emphasisColors, ...siteVars.naturalColors },
typeof mapper === 'number' ? String(mapper) : (mapper as any),
) as ColorValues<T>

export const getColorSchemeFn = <T>(colorProp: string, colorScheme: ColorValues<T>) => {
const colors = _.get(colorScheme, colorProp)
return (area: keyof T, defaultColor: string) => (colors ? colors[area] : defaultColor)
}

export const getColorSchemeFromObject = (
colorScheme: ColorValues<Partial<ColorScheme>>,
colors: ComplexColorPropType,
): Partial<ColorScheme> =>
_.mapValues(colors, (color, colorName) => {
// if the color scheme contains the color, then get the value from it, otherwise return the color provided
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// 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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const colorSchemeValue = _.get(colorScheme, color, colorScheme.default[color])
return colorSchemeValue ? colorSchemeValue[colorName] : colors[colorName]
})

export const getColorSchemeWithCustomDefaults = (
colorScheme: ColorSchemeMapping,
customDefaultValues: Partial<ColorScheme>,
) => {
const mergedDefaultValues = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

...colorScheme.default,
...customDefaultValues,
}
return {
...colorScheme,
default: mergedDefaultValues,
}
}

export const generateColorScheme = (
colorProp: ComplexColorPropType,
colorScheme: ColorValues<Partial<ColorScheme>>,
): Partial<ColorScheme> => {
// if both color prop and color scheme are defined, we are merging them
if (colorProp && colorScheme) {
return typeof colorProp === 'string'
? _.get(colorScheme, colorProp as string, colorScheme.default)
: { ...colorScheme.default, ...getColorSchemeFromObject(colorScheme, colorProp) }
}

// if the color prop is not defined, but the the color scheme is defined, then we are returning
// the defaults from the color scheme if they exists
if (colorScheme) {
return colorScheme && colorScheme.default ? colorScheme.default : {}
}

// if the color scheme is not defined, then if the color prop is a scheme object we are
// returning it, otherwise we return an empty object, as it means that the component is
// implementing the simple color prop
if (colorProp) {
return typeof colorProp === 'string' ? {} : colorProp
}

// if neither the color prop, nor the color scheme are defined, we are returning empty object
return {}
}
39 changes: 25 additions & 14 deletions src/lib/commonPropInterfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,32 @@ export interface UIComponentProps<P = any, V = any>
className?: string
}

export interface ColorComponentProps {
export type ColorValue =
| 'primary'
| 'secondary'
| 'blue'
| 'green'
| 'grey'
| 'orange'
| 'pink'
| 'purple'
| 'teal'
| 'red'
| 'yellow'
| string

export type ComplexColorPropType =
| {
foreground?: ColorValue
background?: ColorValue
border?: ColorValue
shadow?: ColorValue
}
| ColorValue

export interface ColorComponentProps<TColor = ColorValue> {
/** A component can have a color. */
color?:
| 'primary'
| 'secondary'
| 'blue'
| 'green'
| 'grey'
| 'orange'
| 'pink'
| 'purple'
| 'teal'
| 'red'
| 'yellow'
| string
color?: TColor
}

export interface ContentComponentProps<TContent = React.ReactNode> {
Expand Down
46 changes: 29 additions & 17 deletions src/lib/commonPropTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,38 @@ export interface CreateCommonConfig {
children?: boolean | 'node' | 'element'
as?: boolean
className?: boolean
color?: boolean
color?: boolean | 'simple' | 'complex'
content?: boolean | 'node' | 'shorthand'
styled?: boolean
}

const colorPropType = PropTypes.oneOfType([
PropTypes.oneOf([
'primary',
'secondary',
'blue',
'green',
'grey',
'orange',
'pink',
'purple',
'teal',
'red',
'yellow',
]),
PropTypes.string,
])

export const complexColorPropType = PropTypes.oneOfType([
PropTypes.shape({
foreground: colorPropType,
background: colorPropType,
border: colorPropType,
shadow: colorPropType,
}),
colorPropType,
])

export const createCommon = (config: CreateCommonConfig = {}) => {
const {
animated = true,
Expand All @@ -35,22 +62,7 @@ export const createCommon = (config: CreateCommonConfig = {}) => {
className: PropTypes.string,
}),
...(color && {
color: PropTypes.oneOfType([
PropTypes.oneOf([
'primary',
'secondary',
'blue',
'green',
'grey',
'orange',
'pink',
'purple',
'teal',
'red',
'yellow',
]),
PropTypes.string,
]),
color: color === true || color === 'simple' ? colorPropType : complexColorPropType,
}),
...(content && {
content:
Expand Down
2 changes: 1 addition & 1 deletion src/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as commonPropTypes from './commonPropTypes'

export { default as AutoControlledComponent } from './AutoControlledComponent'
export { default as childrenExist } from './childrenExist'
export { mapColorsToScheme } from './colorUtils'
export * from './colorUtils'
export { default as UIComponent } from './UIComponent'
export { EventStack } from './eventStack'
export { felaRenderer, felaRtlRenderer } from './felaRenderer'
Expand Down
6 changes: 6 additions & 0 deletions src/lib/renderComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { mergeComponentStyles, mergeComponentVariables } from './mergeThemes'
import { FocusZoneProps, FocusZone, FocusZone as FabricFocusZone } from './accessibility/FocusZone'
import { FOCUSZONE_WRAP_ATTRIBUTE } from './accessibility/FocusZone/focusUtilities'
import createAnimationStyles from './createAnimationStyles'
import { generateColorScheme } from './index'

export interface RenderResultConfig<P> {
// TODO: Switch back to React.ReactType after issue will be resolved
Expand Down Expand Up @@ -159,6 +160,7 @@ const renderComponent = <P extends {}>(config: RenderConfig<P>): React.ReactElem

const {
siteVariables = {
colorScheme: {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

colors: {},
contextualColors: {},
emphasisColors: {},
Expand Down Expand Up @@ -202,10 +204,14 @@ const renderComponent = <P extends {}>(config: RenderConfig<P>): React.ReactElem
{ handledProps: [...handledProps, ...accessibility.handledProps] },
props,
)

const colors = generateColorScheme(stateAndProps.color, resolvedVariables.colorScheme)

const styleParam: ComponentStyleFunctionParam = {
props: stateAndProps,
variables: resolvedVariables,
theme,
colors,
}

mergedStyles.root = {
Expand Down
41 changes: 38 additions & 3 deletions src/themes/base/colors.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
import { ColorPalette, ContextualColors, EmphasisColors, NaturalColors } from '../types'
import * as _ from 'lodash'

import {
ColorPalette,
ContextualColors,
EmphasisColors,
NaturalColors,
ColorSchemeMapping,
} from '../types'

export const naturalColors: NaturalColors = {
blue: {
Expand Down Expand Up @@ -135,11 +143,38 @@ export const contextualColors: ContextualColors = {
warning: naturalColors.yellow,
}

export const colors: ColorPalette = {
...contextualColors,
const emphasisAndNaturalColors: EmphasisColors & NaturalColors = {
...emphasisColors,
...naturalColors,
}

const lightBackgroundColors = ['teal', 'yellow']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuzhelov agreed, what is your proposal?

add background as a key to the ColorVariants type?

Copy link
Contributor

@kuzhelov kuzhelov Jan 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@mnajdova mnajdova Jan 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const isLightBackground = (colorName: string) => _.includes(lightBackgroundColors, colorName)

export const colors: ColorPalette = {
...emphasisAndNaturalColors,
...contextualColors,

black: '#000',
white: '#fff',
}

export const colorScheme: ColorSchemeMapping = _.mapValues(
emphasisAndNaturalColors,
(colorVariants, colorName) => {
const foreground = isLightBackground(colorName) ? colors.black : colorVariants[50]

return {
foreground,
border: foreground,
shadow: foreground,
background: colorVariants[500],
default: {
foreground: colors.grey[600],
border: colors.grey[600],
shadow: colors.grey[600],
background: colors.grey[100],
},
}
},
)
2 changes: 1 addition & 1 deletion src/themes/base/siteVariables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// COLORS
//

export { colors, contextualColors, emphasisColors, naturalColors } from './colors'
export { colors, contextualColors, emphasisColors, naturalColors, colorScheme } from './colors'

//
// FONT SIZES
Expand Down
Loading