From fc0731b233cc911b2920744de01c1e908733f511 Mon Sep 17 00:00:00 2001 From: Dominic McPhee Date: Thu, 22 Oct 2020 10:39:26 -0400 Subject: [PATCH 1/5] Optimize ThemeProvider custom properties output --- playground/DetailsPage.tsx | 29 ++++++++++--------- .../ThemeProvider/ThemeProvider.tsx | 14 ++++++++- src/utilities/theme/index.ts | 1 + src/utilities/theme/utils.ts | 2 +- 4 files changed, 31 insertions(+), 15 deletions(-) diff --git a/playground/DetailsPage.tsx b/playground/DetailsPage.tsx index b8a85463ea9..3bc509ac43d 100644 --- a/playground/DetailsPage.tsx +++ b/playground/DetailsPage.tsx @@ -35,6 +35,7 @@ import { Stack, TextContainer, TextField, + ThemeProvider, Thumbnail, Toast, TopBar, @@ -610,19 +611,21 @@ export function DetailsPage() { ); return ( - - {contextualSaveBarMarkup} - {loadingMarkup} - {pageMarkup} - {toastMarkup} - {modalMarkup} - + + + {contextualSaveBarMarkup} + {loadingMarkup} + {pageMarkup} + {toastMarkup} + {modalMarkup} + + ); } diff --git a/src/components/ThemeProvider/ThemeProvider.tsx b/src/components/ThemeProvider/ThemeProvider.tsx index 7e87ab63ac8..68f4da594e8 100644 --- a/src/components/ThemeProvider/ThemeProvider.tsx +++ b/src/components/ThemeProvider/ThemeProvider.tsx @@ -6,6 +6,7 @@ import { ThemeConfig, buildThemeContext, buildCustomProperties, + toString, Tokens, } from '../../utilities/theme'; import {useFeatures} from '../../utilities/features'; @@ -84,7 +85,18 @@ export function ThemeProvider({ } }, [backgroundColor, color, isParentThemeProvider]); - const style = {...customProperties, ...(!isParentThemeProvider && {color})}; + let style; + + if (isParentThemeProvider) { + style = customProperties; + } else if ( + !isParentThemeProvider && + parentContext!.cssCustomProperties !== toString(customProperties) + ) { + style = {...customProperties, ...{color}}; + } else { + style = {color}; + } return ( diff --git a/src/utilities/theme/index.ts b/src/utilities/theme/index.ts index 5b8a24a778c..a5b4568775a 100644 --- a/src/utilities/theme/index.ts +++ b/src/utilities/theme/index.ts @@ -8,6 +8,7 @@ export { buildCustomProperties, buildThemeContext, toCssCustomPropertySyntax, + toString, } from './utils'; export * from './tokens'; diff --git a/src/utilities/theme/utils.ts b/src/utilities/theme/utils.ts index eaf99769407..1b058d5b2e2 100644 --- a/src/utilities/theme/utils.ts +++ b/src/utilities/theme/utils.ts @@ -55,7 +55,7 @@ export function buildThemeContext( }; } -function toString(obj?: CustomPropertiesLike) { +export function toString(obj?: CustomPropertiesLike) { if (obj) { return Object.entries(obj) .map((pair) => pair.join(':')) From e4e702ae72267b6d2774da9d478b16bcf7917deb Mon Sep 17 00:00:00 2001 From: Dominic McPhee Date: Thu, 22 Oct 2020 16:44:04 -0400 Subject: [PATCH 2/5] Updating memoization --- playground/DetailsPage.tsx | 29 ++++++------ .../ThemeProvider/ThemeProvider.tsx | 44 +++++++++++-------- .../tests/ThemeProvider.test.tsx | 6 +-- 3 files changed, 42 insertions(+), 37 deletions(-) diff --git a/playground/DetailsPage.tsx b/playground/DetailsPage.tsx index 3bc509ac43d..b8a85463ea9 100644 --- a/playground/DetailsPage.tsx +++ b/playground/DetailsPage.tsx @@ -35,7 +35,6 @@ import { Stack, TextContainer, TextField, - ThemeProvider, Thumbnail, Toast, TopBar, @@ -611,21 +610,19 @@ export function DetailsPage() { ); return ( - - - {contextualSaveBarMarkup} - {loadingMarkup} - {pageMarkup} - {toastMarkup} - {modalMarkup} - - + + {contextualSaveBarMarkup} + {loadingMarkup} + {pageMarkup} + {toastMarkup} + {modalMarkup} + ); } diff --git a/src/components/ThemeProvider/ThemeProvider.tsx b/src/components/ThemeProvider/ThemeProvider.tsx index 68f4da594e8..5296e881e90 100644 --- a/src/components/ThemeProvider/ThemeProvider.tsx +++ b/src/components/ThemeProvider/ThemeProvider.tsx @@ -39,15 +39,16 @@ export function ThemeProvider({ const parentContext = useContext(ThemeContext); const isParentThemeProvider = parentContext === undefined; - const processedThemeConfig = useMemo(() => { - const parentColorScheme = - parentContext && parentContext.colorScheme && parentContext.colorScheme; - const parentColors = - parentContext && parentContext.colors && parentContext.colors; + const parentColorScheme = + parentContext && parentContext.colorScheme && parentContext.colorScheme; + const parentColors = + parentContext && parentContext.colors && parentContext.colors; + + const [customProperties, theme] = useMemo(() => { const {colors, colorScheme, ...rest} = themeConfig; - return { + const processedThemeConfig = { ...rest, ...{colorScheme: getColorScheme(colorScheme, parentColorScheme)}, colors: { @@ -56,22 +57,29 @@ export function ThemeProvider({ ...colors, }, }; - }, [parentContext, themeConfig, isParentThemeProvider]); - const customProperties = useMemo( - () => - buildCustomProperties(processedThemeConfig, newDesignLanguage, Tokens), - [processedThemeConfig, newDesignLanguage], - ); + const customProperties = buildCustomProperties( + processedThemeConfig, + newDesignLanguage, + Tokens, + ); - const theme = useMemo( - () => - buildThemeContext( + const theme = { + ...buildThemeContext( processedThemeConfig, newDesignLanguage ? customProperties : undefined, ), - [customProperties, processedThemeConfig, newDesignLanguage], - ); + textColor: customProperties['--p-text'] || '', + }; + + return [customProperties, theme]; + }, [ + isParentThemeProvider, + newDesignLanguage, + parentColorScheme, + parentColors, + themeConfig, + ]); // We want these values to be empty string instead of `undefined` when not set. // Otherwise, setting a style property to `undefined` does not remove it from the DOM. @@ -99,7 +107,7 @@ export function ThemeProvider({ } return ( - +
{children}
 ); diff --git a/src/components/ThemeProvider/tests/ThemeProvider.test.tsx b/src/components/ThemeProvider/tests/ThemeProvider.test.tsx index 4057465a10f..39ebb97b591 100644 --- a/src/components/ThemeProvider/tests/ThemeProvider.test.tsx +++ b/src/components/ThemeProvider/tests/ThemeProvider.test.tsx @@ -189,7 +189,7 @@ describe('', () => { }); describe('when nested', () => { - it('sets a default theme', () => { + it('does not render custom properties if themes are identical', () => { const themeProvider = mountWithNewDesignLanguage( @@ -200,7 +200,7 @@ describe('', () => { ); expect(themeProvider.findAll('div')[1]).toHaveReactProps({ - style: expect.objectContaining({ + style: expect.not.objectContaining({ '--p-background': expect.any(String), '--p-text': expect.any(String), '--p-interactive': expect.any(String), @@ -234,7 +234,7 @@ describe('', () => { ); expect(themeProvider.findAll('div')[1]).toHaveReactProps({ - style: expect.objectContaining({ + style: expect.not.objectContaining({ '--p-surface': 'rgba(255, 255, 255, 1)', }), }); From 7390227fd7d8f2fe0fe93b91d85be0d2ba7cf86a Mon Sep 17 00:00:00 2001 From: Dominic McPhee Date: Fri, 23 Oct 2020 09:21:29 -0400 Subject: [PATCH 3/5] Feedback based on review --- src/components/ThemeProvider/ThemeProvider.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/components/ThemeProvider/ThemeProvider.tsx b/src/components/ThemeProvider/ThemeProvider.tsx index 5296e881e90..025753c40cf 100644 --- a/src/components/ThemeProvider/ThemeProvider.tsx +++ b/src/components/ThemeProvider/ThemeProvider.tsx @@ -101,13 +101,14 @@ export function ThemeProvider({ !isParentThemeProvider && parentContext!.cssCustomProperties !== toString(customProperties) ) { - style = {...customProperties, ...{color}}; + customProperties.color = color; + style = customProperties; } else { style = {color}; } return ( - +
{children}
 ); From 0b02939f3f489c357ec2fd9274920930c7b3c965 Mon Sep 17 00:00:00 2001 From: Dominic McPhee Date: Fri, 23 Oct 2020 09:48:29 -0400 Subject: [PATCH 4/5] Adding to unreleased --- UNRELEASED.md | 4 ++++ src/components/ThemeProvider/ThemeProvider.tsx | 3 +-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/UNRELEASED.md b/UNRELEASED.md index b800fc5c53c..883633c96bf 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -9,6 +9,10 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f ### Enhancements - Updated `MediaCard` to accept ReactNode as title and make `primaryAction` optional ([#3552](https://github.com/Shopify/polaris-react/pull/3552)) +- Updated Textfield with a type of number to not render a spinner if step is set to 0 ([#3477](https://github.com/Shopify/polaris-react/pull/3477)) +- Refactored `Portal` to render all `Portals` in a single container ([#3544](https://github.com/Shopify/polaris-react/pull/3544)) + +- Optimized `ThemeProvider` to only output its custom properties in nested `ThemeProvider`s when they differ from the parent context ([#3550](https://github.com/Shopify/polaris-react/pull/3550)) ### Bug fixes diff --git a/src/components/ThemeProvider/ThemeProvider.tsx b/src/components/ThemeProvider/ThemeProvider.tsx index 025753c40cf..a742a578c3d 100644 --- a/src/components/ThemeProvider/ThemeProvider.tsx +++ b/src/components/ThemeProvider/ThemeProvider.tsx @@ -101,8 +101,7 @@ export function ThemeProvider({ !isParentThemeProvider && parentContext!.cssCustomProperties !== toString(customProperties) ) { - customProperties.color = color; - style = customProperties; + style = {...customProperties, ...{color}}; } else { style = {color}; } From 6fadd2283190f7f5c9f7e076e3fcca21b898148a Mon Sep 17 00:00:00 2001 From: Dominic McPhee Date: Mon, 26 Oct 2020 10:15:49 -0400 Subject: [PATCH 5/5] Updating UNRELEASED --- UNRELEASED.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/UNRELEASED.md b/UNRELEASED.md index 883633c96bf..4ce1c3207ca 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -9,8 +9,6 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f ### Enhancements - Updated `MediaCard` to accept ReactNode as title and make `primaryAction` optional ([#3552](https://github.com/Shopify/polaris-react/pull/3552)) -- Updated Textfield with a type of number to not render a spinner if step is set to 0 ([#3477](https://github.com/Shopify/polaris-react/pull/3477)) -- Refactored `Portal` to render all `Portals` in a single container ([#3544](https://github.com/Shopify/polaris-react/pull/3544)) - Optimized `ThemeProvider` to only output its custom properties in nested `ThemeProvider`s when they differ from the parent context ([#3550](https://github.com/Shopify/polaris-react/pull/3550))