-
Notifications
You must be signed in to change notification settings - Fork 53
feat(Label): add color
prop
#647
Changes from all commits
f123ea6
400415a
79c4c4f
d0aaea2
1f824e4
c2d712c
28156e6
8224fb8
2e9b8c7
a85e544
472c363
77e5562
cb731e2
c5aa276
c98ce09
b2e255a
0e5a91f
233403b
d299f0d
23c44e3
623e727
6cfc122
4b6bf36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. */ | ||
|
@@ -55,7 +58,7 @@ class Label extends UIComponent<ReactProps<LabelProps>, any> { | |
static className = 'ui-label' | ||
|
||
static propTypes = { | ||
...commonPropTypes.createCommon(), | ||
...commonPropTypes.createCommon({ color: 'complex' }), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Always try to delete as much code and work as possible. Consider this line: ...commonPropTypes.createCommon({ color: 'complex' }),
We can achieve this much more simply like this: color: customPropTypes.complexColor
|
||
circular: PropTypes.bool, | ||
icon: customPropTypes.itemShorthand, | ||
iconPosition: PropTypes.oneOf(['start', 'end']), | ||
|
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, | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This idea looks quite confusing to me :( I have a definition for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couple of thoughts on this:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
const colorSchemeValue = _.get(colorScheme, color, colorScheme.default[color]) | ||
return colorSchemeValue ? colorSchemeValue[colorName] : colors[colorName] | ||
}) | ||
|
||
export const getColorSchemeWithCustomDefaults = ( | ||
colorScheme: ColorSchemeMapping, | ||
customDefaultValues: Partial<ColorScheme>, | ||
) => { | ||
const mergedDefaultValues = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
...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 {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -159,6 +160,7 @@ const renderComponent = <P extends {}>(config: RenderConfig<P>): React.ReactElem | |
|
||
const { | ||
siteVariables = { | ||
colorScheme: {}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would be great if we will be able to reduce There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kuzhelov are you suggesting having maybe one There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = {
color: {
scheme,
contextual,
natural,
...
}
...
} This will aid maintainability (as all closely related aspects will be semantically grouped), as well as will not pollute the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
colors: {}, | ||
contextualColors: {}, | ||
emphasisColors: {}, | ||
|
@@ -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 = { | ||
|
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: { | ||
|
@@ -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'] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
We definitely should think on some mechanism that will introduce this ability to select foreground color depending on the background. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kuzhelov agreed, what is your proposal? add There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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], | ||
}, | ||
} | ||
}, | ||
) |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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: