Skip to content

Optimize ThemeProvider custom properties output #3550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Oct 26, 2020
Merged

Optimize ThemeProvider custom properties output #3550

merged 5 commits into from
Oct 26, 2020

Conversation

@dfmcphee
Copy link
Contributor

@dfmcphee dfmcphee commented Oct 22, 2020 •
edited
Loading

This is an alternative approach to #3546.

WHY are these changes introduced?

Currently we output the custom properties on each level of ThemeProvider even if they are identical to the parent.

WHAT is this pull request doing?

This changes the ThemeProvider to compare the generated custom properties with the parent context and only output them if they differ.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

I committed a change to the Details Page example to try this out. If you change to the new design language light mode, you should see only one top level output of the custom properties since the AppProvider's ThemeProvider and the ThemeProvider wrapping the Details Page frame match. If you switch to dark mode you should see it output on both and that the page stays in light mode because of the nested ThemeProvider's custom properties.

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@dfmcphee dfmcphee requested a review from kyledurand October 22, 2020 14:43
@github-actions
Copy link
Contributor

github-actions bot commented Oct 22, 2020 •
edited
Loading

🟡 This pull request modifies 5 files and might impact 65 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:
All files potentially affected (total: 65)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/ThemeProvider/ThemeProvider.tsx (total: 61)

Files potentially affected (total: 61)

🧩 src/components/ThemeProvider/tests/ThemeProvider.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/utilities/theme/index.ts (total: 64)

Files potentially affected (total: 64)

🧩 src/utilities/theme/utils.ts (total: 65)

Files potentially affected (total: 65)

kyledurand
kyledurand approved these changes Oct 22, 2020
View reviewed changes
Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Love it 🥇

Makes things much less confusing for consumers

@BPScott
Copy link
Member

BPScott commented Oct 22, 2020

Tangental: In #3527 I identified that we're recreating the object we store in the ThemeContext on every rerender, and found a smallish fix for it (with a further suggesting that we should replace those multiple useMemo calls with a single useMemo call). Is it worth working at caching this response here or should we deal with that in a separate PR?

@dfmcphee
Copy link
Contributor Author

Is it worth working at caching this response here or should we deal with that in a separate PR?

Good call @BPScott. I want to do a bit of perf testing with this change so it would make sense to try to get that fixed in here as well so we can test it all together.

BPScott
BPScott reviewed Oct 22, 2020
View reviewed changes
BPScott
BPScott reviewed Oct 22, 2020
View reviewed changes
@dfmcphee dfmcphee marked this pull request as ready for review October 23, 2020 13:49
@dfmcphee
Copy link
Contributor Author

Alrighty, this should be ready for a real review now. Following a similar approach to what @BPScott did in #3527 I was able to get this memoizing properly.

One thing I noticed was that the parent context from the AppProvider was always changing in storybook, but when I moved to a new create-react-app to test, it didn't have that issue.

I tested in that app with something like this:

import React, {useState} from 'react';

import {Page, ThemeProvider, Button, Card} from '@shopify/polaris';
const FIRST_THEME = {colorScheme: 'light'};

export default function App() {
  const [baseTheme, setBaseTheme] = useState('light');

  const toggle = () => setBaseTheme(baseTheme === 'light' ? 'dark' : 'light');

  const secondTheme = {colorScheme: baseTheme};
  return (
    <Page title="Playground">
      <Button onClick={toggle}>Toggle child theme</Button>
      <ThemeProvider theme={FIRST_THEME}>
        <Card sectioned>First themed card</Card>
        <ThemeProvider theme={secondTheme}>
          <Card sectioned>Second themed card</Card>
        </ThemeProvider>
      </ThemeProvider>
    </Page>
  );
}

The 3 theme providers have their values memoized on first render and when clicking the toggle button, only the most nested provider reruns the memoization.

The custom properties also only output when the secondTheme is in dark mode.

@dfmcphee
Copy link
Contributor Author

One other thing I noticed in that example. If you declare firstTheme inside the App function, or declare it inline in the props, it will also try to rememoize every render because that object gets recreated each time. This is something we should look out for in web if we can too.

@dfmcphee dfmcphee force-pushed the optimize-custom-properties-output branch from 885a053 to 96785f4 Compare October 23, 2020 14:01
@dfmcphee
Copy link
Contributor Author

This fixes #3448

@dfmcphee dfmcphee requested a review from martenbjork October 23, 2020 14:25
@dfmcphee dfmcphee force-pushed the optimize-custom-properties-output branch from 96785f4 to 6fadd22 Compare October 26, 2020 14:16
@dfmcphee dfmcphee merged commit 93d0727 into master Oct 26, 2020
@dfmcphee dfmcphee deleted the optimize-custom-properties-output branch October 26, 2020 14:56
sylvhama pushed a commit that referenced this pull request Mar 26, 2021
@sylvhama
Optimize ThemeProvider custom properties output (#3550)
b2e0faa 
* Optimize ThemeProvider custom properties output

* Updating memoization

* Feedback based on review

* Adding to unreleased

* Updating UNRELEASED
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BPScott BPScott BPScott left review comments

@kyledurand kyledurand kyledurand approved these changes

@martenbjork martenbjork Awaiting requested review from martenbjork

Assignees

@alex-page alex-page

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dfmcphee @BPScott @kyledurand @alex-page