diff --git a/UNRELEASED.md b/UNRELEASED.md index 7168ac76d21..ca91b6a9c1a 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -22,6 +22,17 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f ### Code quality +- Improved code quality for the theme provider component ([#2225](https://github.com/Shopify/polaris-react/pull/2225)): + + - updated type for `theme` prop to `ThemeConfig` to distinguish from the type `Theme` which is shared over context. A `Theme` contains only the logo properties, while `ThemeConfig` can contain a `colors` property. + - converted `ThemeProvider` to use hooks + - created symmetry in context between app provider and test provider + - added better tests for default topBar colors + - fixed an issue where `colorToHsla` returned HSLA strings instead of HSLA objects when given HSL or HSLA strings + - fixed an issue with `colorToHsla` where RGB colors with no saturation could result in a divide by zero error + - fixed an issue where `colorToHsla` inconsistently returned an alpha value + - fixed an issue where `lightenColor` and `darkenColor` would lighten or darken absolute lightness vales (0, 100) + ### Deprecations ### Development workflow diff --git a/src/components/AppProvider/AppProvider.tsx b/src/components/AppProvider/AppProvider.tsx index bf282f9f40f..e9ece650668 100644 --- a/src/components/AppProvider/AppProvider.tsx +++ b/src/components/AppProvider/AppProvider.tsx @@ -1,5 +1,5 @@ import React from 'react'; -import {Theme} from '../../utilities/theme'; +import {ThemeConfig} from '../../utilities/theme'; import {ThemeProvider} from '../ThemeProvider'; import {MediaQueryProvider} from '../MediaQueryProvider'; import {I18n, I18nContext, TranslationDictionary} from '../../utilities/i18n'; @@ -36,7 +36,7 @@ export interface AppProviderProps extends AppBridgeOptions { /** A custom component to use for all links used by Polaris components */ linkComponent?: LinkLikeComponent; /** Custom logos and colors provided to select components */ - theme?: Theme; + theme?: ThemeConfig; /** For toggling features */ features?: Features; /** Inner content of the application */ @@ -98,7 +98,7 @@ export class AppProvider extends React.Component { } render() { - const {theme = {logo: null}, features = {}, children} = this.props; + const {theme = {}, features = {}, children} = this.props; const {intl, appBridge, link} = this.state; return ( diff --git a/src/components/PolarisTestProvider/PolarisTestProvider.tsx b/src/components/PolarisTestProvider/PolarisTestProvider.tsx index d6a9d0c517f..91798c300a7 100644 --- a/src/components/PolarisTestProvider/PolarisTestProvider.tsx +++ b/src/components/PolarisTestProvider/PolarisTestProvider.tsx @@ -1,7 +1,11 @@ import React from 'react'; import {merge} from '../../utilities/merge'; import {FrameContext} from '../../utilities/frame'; -import {Theme, ThemeContext} from '../../utilities/theme'; +import { + ThemeContext, + ThemeConfig, + buildThemeContext, +} from '../../utilities/theme'; import {MediaQueryContext} from '../../utilities/media-query'; import { ScrollLockManager, @@ -36,7 +40,7 @@ export type WithPolarisTestProviderOptions = { i18n?: TranslationDictionary | TranslationDictionary[]; appBridge?: AppBridgeOptions; link?: LinkLikeComponent; - theme?: Partial; + theme?: ThemeConfig; mediaQuery?: Partial; features?: Features; // Contexts provided by Frame @@ -59,7 +63,7 @@ export function PolarisTestProvider({ i18n, appBridge, link, - theme, + theme = {}, mediaQuery, features = {}, frame, @@ -78,7 +82,7 @@ export function PolarisTestProvider({ // I'm not that worried about it const appBridgeApp = appBridge as React.ContextType; - const mergedTheme = createThemeContext(theme); + const mergedTheme = buildThemeContext(theme); const mergedFrame = createFrameContext(frame); @@ -113,11 +117,6 @@ export function PolarisTestProvider({ function noop() {} -function createThemeContext(theme: Partial = {}): Theme { - const {logo = null} = theme; - return {logo}; -} - function createFrameContext({ showToast = noop, hideToast = noop, diff --git a/src/components/ThemeProvider/ThemeProvider.tsx b/src/components/ThemeProvider/ThemeProvider.tsx index b8edc5c8a4d..bd611d9967d 100644 --- a/src/components/ThemeProvider/ThemeProvider.tsx +++ b/src/components/ThemeProvider/ThemeProvider.tsx @@ -1,77 +1,33 @@ -import React from 'react'; -import isEqual from 'lodash/isEqual'; -import {ThemeContext} from '../../utilities/theme'; -import {Theme} from '../../utilities/theme/types'; -import {setColors} from '../../utilities/theme/utils'; +import React, {useMemo} from 'react'; +import { + ThemeContext, + ThemeConfig, + buildThemeContext, + buildCustomProperties, +} from '../../utilities/theme'; import {themeProvider} from '../shared'; -interface State { - theme: Theme; - colors: string[][] | undefined; -} - interface ThemeProviderProps { /** Custom logos and colors provided to select components */ - theme: Theme; + theme: ThemeConfig; /** The content to display */ children?: React.ReactNode; } -const defaultTheme = { - '--top-bar-background': '#00848e', - '--top-bar-color': '#f9fafb', - '--top-bar-background-lighter': '#1d9ba4', -}; - -export class ThemeProvider extends React.Component { - state: State = { - theme: setThemeContext(this.props.theme), - colors: setColors(this.props.theme), - }; - - componentDidUpdate({theme: prevTheme}: ThemeProviderProps) { - const {theme} = this.props; - if (isEqual(prevTheme, theme)) { - return; - } - - // eslint-disable-next-line react/no-did-update-set-state - this.setState({ - theme: setThemeContext(theme), - colors: setColors(theme), - }); - } - - render() { - const { - theme: {logo = null, ...rest}, - } = this.state; - const {children} = this.props; - const styles = this.createStyles() || defaultTheme; - - const theme = { - ...rest, - logo, - }; - - return ( - -
- {children} -
-
- ); - } - - createStyles() { - const {colors} = this.state; - return colors - ? colors.reduce((state, [key, value]) => ({...state, [key]: value}), {}) - : null; - } -} - -function setThemeContext(ctx: Theme): Theme { - const {colors, ...theme} = ctx; - return {...theme}; +export function ThemeProvider({ + theme: themeConfig, + children, +}: ThemeProviderProps) { + const theme = useMemo(() => buildThemeContext(themeConfig), [themeConfig]); + const customProperties = useMemo(() => buildCustomProperties(themeConfig), [ + themeConfig, + ]); + + return ( + +
+ {children} +
+
+ ); } diff --git a/src/components/ThemeProvider/tests/ThemeProvider.test.tsx b/src/components/ThemeProvider/tests/ThemeProvider.test.tsx index da1b0ace6d2..04ffed09782 100644 --- a/src/components/ThemeProvider/tests/ThemeProvider.test.tsx +++ b/src/components/ThemeProvider/tests/ThemeProvider.test.tsx @@ -6,7 +6,7 @@ import {ThemeContext} from '../../../utilities/theme'; describe('', () => { it('mounts', () => { const themeProvider = mountWithAppProvider( - +

Hello

, ); @@ -41,10 +41,7 @@ describe('', () => { , ); - const div = wrapper - .find(Child) - .find('div') - .first(); + const div = wrapper.find(Child).find('div'); expect(div.exists()).toBe(true); }); @@ -56,7 +53,11 @@ describe('', () => { , ); - expect(wrapper.find('div').props().style).toBeDefined(); + expect(wrapper.find('div').props().style).toStrictEqual({ + '--top-bar-background': '#00848e', + '--top-bar-background-lighter': '#f9fafb', + '--top-bar-color': '#1d9ba4', + }); }); it('sets a provided theme', () => { diff --git a/src/utilities/color-manipulation.ts b/src/utilities/color-manipulation.ts index 08038f7fc58..8fccb55690b 100644 --- a/src/utilities/color-manipulation.ts +++ b/src/utilities/color-manipulation.ts @@ -1,3 +1,4 @@ +import {clamp} from '@shopify/javascript-utilities/math'; import {HSLColor, HSBColor} from './color-types'; export function lightenColor(color: HSLColor | string, lighten = 0) { @@ -8,7 +9,7 @@ export function lightenColor(color: HSLColor | string, lighten = 0) { const {lightness} = color; const nextLightness = lightness + lighten; - return {...color, lightness: nextLightness}; + return {...color, lightness: clamp(nextLightness, 0, 100)}; } export function darkenColor(color: HSLColor | string, lighten = 0) { @@ -19,7 +20,7 @@ export function darkenColor(color: HSLColor | string, lighten = 0) { const {lightness} = color; const nextLightness = lightness - lighten; - return {...color, lightness: nextLightness}; + return {...color, lightness: clamp(nextLightness, 0, 100)}; } export function saturateColor( diff --git a/src/utilities/color-transformers.ts b/src/utilities/color-transformers.ts index f607233cc55..bbc630ce168 100644 --- a/src/utilities/color-transformers.ts +++ b/src/utilities/color-transformers.ts @@ -147,10 +147,11 @@ function rgbToHsbl(color: RGBAColor, type: 'b' | 'l' = 'b'): HSBLAColor { } else if (type === 'b') { saturation = delta / largestComponent; } else if (type === 'l') { - saturation = + const baseSaturation = lightness > 0.5 ? delta / (2 - largestComponent - smallestComponent) : delta / (largestComponent + smallestComponent); + saturation = isNaN(baseSaturation) ? 0 : baseSaturation; } let huePercentage = 0; @@ -177,20 +178,18 @@ function rgbToHsbl(color: RGBAColor, type: 'b' | 'l' = 'b'): HSBLAColor { } export function rgbToHsb(color: RGBColor): HSBColor; -export function rgbToHsb(color: RGBAColor): HSBAColor; export function rgbToHsb(color: RGBAColor): HSBAColor { - const {hue, saturation, brightness, alpha} = rgbToHsbl(color, 'b'); + const {hue, saturation, brightness, alpha = 1} = rgbToHsbl(color, 'b'); return {hue, saturation, brightness, alpha}; } -export function rgbToHsl(color: RGBColor): HSLColor; -export function rgbToHsl(color: RGBAColor): HSLAColor; +export function rgbToHsl(color: RGBColor): HSLAColor; export function rgbToHsl(color: RGBAColor): HSLAColor { const { hue, saturation: rawSaturation, lightness: rawLightness, - alpha, + alpha = 1, } = rgbToHsbl(color, 'l'); const saturation = rawSaturation * 100; const lightness = rawLightness * 100; @@ -253,7 +252,7 @@ export function hslToString(hslColor: HSLAColor | string) { return `hsl(${hue}, ${saturation}%, ${lightness}%, ${alpha})`; } -function rgbToObject(color: string) { +function rgbToObject(color: string): RGBAColor { const colorMatch = color.match(/\(([^)]+)\)/); if (!colorMatch) { @@ -270,27 +269,48 @@ function rgbToObject(color: string) { return objColor; } -const hexToHsl: (color: string) => HSLColor | HSLAColor = compose( +const hexToHsla: (color: string) => HSLAColor = compose( rgbToHsl, hexToRgb, ); -const rbgStringToHsl: (color: string) => HSLColor | HSLAColor = compose( +const rbgStringToHsla: (color: string) => HSLAColor = compose( rgbToHsl, rgbToObject, ); -export function colorToHsla(color: string) { +function hslToObject(color: string): HSLAColor { + const colorMatch = color.match(/\(([^)]+)\)/); + + if (!colorMatch) { + return {hue: 0, saturation: 0, lightness: 0, alpha: 0}; + } + + const [hue, saturation, lightness, alpha] = colorMatch[1].split(','); + const objColor = { + hue: parseInt(hue, 10), + saturation: parseInt(saturation, 10), + lightness: parseInt(lightness, 10), + alpha: parseFloat(alpha) || 1, + }; + return objColor; +} + +export function colorToHsla(color: string): HSLAColor { const type: ColorType = getColorType(color); switch (type) { case ColorType.Hex: - return hexToHsl(color); + return hexToHsla(color); case ColorType.Rgb: case ColorType.Rgba: - return rbgStringToHsl(color); - case ColorType.Hsl: + return rbgStringToHsla(color); case ColorType.Hsla: + case ColorType.Hsl: + return hslToObject(color); + case ColorType.Default: default: - return color; + throw new Error( + 'Accepted color formats are: hex, rgb, rgba, hsl and hsla', + ); } } diff --git a/src/utilities/tests/color-manipulation.test.ts b/src/utilities/tests/color-manipulation.test.ts index e3448372a56..54147018310 100644 --- a/src/utilities/tests/color-manipulation.test.ts +++ b/src/utilities/tests/color-manipulation.test.ts @@ -20,6 +20,11 @@ describe('lightenColor', () => { ); expect((lightColor as HSLColor).lightness).toBeGreaterThan(50); }); + + it('returns a valid color when an input is at maximum lightness', () => { + const lightColor = lightenColor({hue: 0, saturation: 0, lightness: 100}, 5); + expect((lightColor as HSLColor).lightness).toBe(100); + }); }); describe('darkenColor', () => { @@ -32,6 +37,11 @@ describe('darkenColor', () => { const darkColor = darkenColor({hue: 50, saturation: 50, lightness: 50}, 5); expect((darkColor as HSLColor).lightness).toBeLessThan(50); }); + + it('returns a valid color when an input is at maximum lightness', () => { + const darkColor = darkenColor({hue: 0, saturation: 0, lightness: 0}, 5); + expect((darkColor as HSLColor).lightness).toBe(0); + }); }); describe('saturateColor', () => { diff --git a/src/utilities/tests/color-transformers.test.ts b/src/utilities/tests/color-transformers.test.ts index cb2e6219b01..456641e428a 100644 --- a/src/utilities/tests/color-transformers.test.ts +++ b/src/utilities/tests/color-transformers.test.ts @@ -126,6 +126,15 @@ describe('colorUtilities', () => { }); }); + it('returns a valid color when given an rgb color with no saturation', () => { + expect(colorToHsla('rgb(0, 0, 0)')).toStrictEqual({ + alpha: 1, + hue: 0, + lightness: 0, + saturation: 0, + }); + }); + it('returns the hsla color for rgba', () => { expect(colorToHsla('rgb(132, 11, 2, 0.2)')).toStrictEqual({ alpha: 1, @@ -136,13 +145,21 @@ describe('colorUtilities', () => { }); it('returns the hsla color for hsl', () => { - expect(colorToHsla('hsla(120, 100%, 50%)')).toBe('hsla(120, 100%, 50%)'); + expect(colorToHsla('hsla(120, 100%, 50%)')).toStrictEqual({ + alpha: 1, + hue: 120, + lightness: 50, + saturation: 100, + }); }); it('returns the hsla color for hsla', () => { - expect(colorToHsla('hsla(120, 100%, 50%, 0.3)')).toBe( - 'hsla(120, 100%, 50%, 0.3)', - ); + expect(colorToHsla('hsla(120, 100%, 50%, 0.3)')).toStrictEqual({ + alpha: 0.3, + hue: 120, + lightness: 50, + saturation: 100, + }); }); }); }); diff --git a/src/utilities/theme/index.ts b/src/utilities/theme/index.ts index 6d247caf809..f1cfeb3aa5d 100644 --- a/src/utilities/theme/index.ts +++ b/src/utilities/theme/index.ts @@ -2,4 +2,6 @@ export {ThemeContext} from './context'; export {useTheme} from './hooks'; -export {Theme} from './types'; +export {Theme, ThemeConfig} from './types'; + +export {buildCustomProperties, buildThemeContext} from './utils'; diff --git a/src/utilities/theme/tests/utils.test.ts b/src/utilities/theme/tests/utils.test.ts index c55ec055d1f..a59bf2bb9be 100644 --- a/src/utilities/theme/tests/utils.test.ts +++ b/src/utilities/theme/tests/utils.test.ts @@ -1,7 +1,12 @@ import tokens from '@shopify/polaris-tokens'; - -import {needsVariant, setColors, setTextColor, setTheme} from '../utils'; import {needsVariantList} from '../config'; +import { + needsVariant, + setTextColor, + setTheme, + buildThemeContext, + buildCustomProperties, +} from '../utils'; describe('setTextColor', () => { it('sets a css variable to white if the variant is dark', () => { @@ -43,11 +48,21 @@ describe('needsVariant', () => { }); }); -describe('setColors', () => { - it('iterates over colors when a theme is passed', () => { +describe('buildCustomProperties', () => { + it('builds an object of css custom properties and colors for a given theme', () => { const theme = {colors: {topBar: {background: '#eeeeee'}}}; - const spy = jest.spyOn(Object, 'entries'); - setColors(theme); - expect(spy).toHaveBeenCalledWith(theme.colors); + + const colors = buildCustomProperties(theme); + expect(colors).toStrictEqual({ + '--top-bar-background': '#eeeeee', + '--top-bar-background-lighter': 'hsl(0, 10%, 100%, 1)', + '--top-bar-color': 'rgb(33, 43, 54)', + }); + }); +}); + +describe('buildThemeContext', () => { + it('reduces theme config down to a theme', () => { + expect(buildThemeContext({colors: {}, logo: {}})).toStrictEqual({logo: {}}); }); }); diff --git a/src/utilities/theme/types.ts b/src/utilities/theme/types.ts index 8838f82a6c1..3ee150e0c1b 100644 --- a/src/utilities/theme/types.ts +++ b/src/utilities/theme/types.ts @@ -1,5 +1,3 @@ -export type ColorsToParse = ThemeColor; - export type ThemeLogo = { /** Provides a path for a logo used on a dark background */ topBarSource?: string; @@ -11,25 +9,22 @@ export type ThemeLogo = { accessibilityLabel?: string; /** Number of pixels wide the logo image is */ width?: number; -} | null; - -export interface ThemeColor { - [key: string]: string; -} +}; -export interface TopBar extends ThemeColor { - background: string; +// The value that is passed into the ThemeProvider +export interface ThemeConfig { + /** Sets the logo for the top bar and contextual save bar components*/ + logo?: ThemeLogo; + colors?: { + /** Sets the background color of the top bar component. Complimentary and typography colors are determined programmatically */ + topBar?: Record; + }; } -export type ThemeColors = { - topBar: TopBar; -}; - +// The value that is stored in the ThemeContext export interface Theme { /** Sets the logo for the top bar and contextual save bar components*/ logo?: ThemeLogo; - /** Sets the background color of the top bar component. Complimentary and typography colors are determined programmatically */ - colors?: ThemeColors; } -export type ThemeVariant = 'light' | 'dark'; +export type CustomPropertiesLike = Record; diff --git a/src/utilities/theme/utils.ts b/src/utilities/theme/utils.ts index 15cce1b1bb8..c1ba4c302e1 100644 --- a/src/utilities/theme/utils.ts +++ b/src/utilities/theme/utils.ts @@ -5,27 +5,44 @@ import {isLight} from '../color-validation'; import {constructColorName} from '../color-names'; import {createLightColor} from '../color-manipulation'; import {compose} from '../compose'; - -import {Theme, ColorsToParse, ThemeVariant, ThemeColors} from './types'; import {needsVariantList} from './config'; +import {ThemeConfig, Theme, CustomPropertiesLike} from './types'; + +export function buildCustomProperties( + themeConfig: ThemeConfig, +): CustomPropertiesLike { + return { + ...setColors(themeConfig), + }; +} -export function setColors(theme: Theme | undefined): string[][] | undefined { +export function buildThemeContext(themeConfig: ThemeConfig): Theme { + const {logo} = themeConfig; + return {logo}; +} + +function setColors(theme: ThemeConfig): CustomPropertiesLike { let colorPairs; - if (theme && theme.colors) { - Object.entries(theme.colors).forEach(([colorKey, pairs]) => { - const colorKeys = Object.keys(pairs); - if (colorKey === 'topBar' && colorKeys.length > 1) { - colorPairs = colorKeys.map((key: string) => { - const colors = (theme.colors as ThemeColors).topBar; - return [constructColorName(colorKey, key), colors[key]]; - }); - } else { - colorPairs = parseColors([colorKey, pairs]); - } + const colors = + theme && theme.colors && theme.colors.topBar + ? theme.colors.topBar + : {background: '#00848e', backgroundLighter: '#f9fafb', color: '#1d9ba4'}; + + const colorKey = 'topBar'; + const colorKeys = Object.keys(colors); + + if (colorKeys.length > 1) { + colorPairs = colorKeys.map((key) => { + return [constructColorName(colorKey, key), colors[key]]; }); + } else { + colorPairs = parseColors([colorKey, colors]); } - return colorPairs; + return colorPairs.reduce( + (state, [key, value]) => ({...state, [key]: value}), + {}, + ); } export function needsVariant(name: string) { @@ -43,7 +60,7 @@ const lightenToString: ( export function setTextColor( name: string, - variant: ThemeVariant = 'dark', + variant: 'light' | 'dark' = 'dark', ): string[] { if (variant === 'light') { return [name, tokens.colorInk]; @@ -88,7 +105,10 @@ export function setTheme( return colorPairs; } -function parseColors([baseName, colors]: [string, ColorsToParse]): string[][] { +function parseColors([baseName, colors]: [ + string, + {[key: string]: string}, +]): string[][] { const keys = Object.keys(colors); const colorPairs = []; for (let i = 0; i < keys.length; i++) {