Skip to content

[ThemeProvider] Add prop to disable rendering of custom properties #3546

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
Closed
wants to merge 1 commit into from
Closed

[ThemeProvider] Add prop to disable rendering of custom properties #3546

wants to merge 1 commit into from

Conversation

@kyledurand
Copy link
Member

@kyledurand kyledurand commented Oct 21, 2020 •
edited
Loading

WHY are these changes introduced?

Fixes an issue in web where we don't always need to pass custom properties to the theme provider node

We initially had two identical providers, one from AppProvider and one from ThemeProvider rendering two sets of custom properties but adding no value. This gives us control over when we need these properties to render

image

WHAT is this pull request doing?

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx: 
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 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

@kyledurand kyledurand added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Oct 21, 2020
@github-actions
Copy link
Contributor

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

🟡 This pull request modifies 2 files and might impact 61 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: 61)
🧩 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)

@kyledurand kyledurand requested a review from dfmcphee October 21, 2020 23:29
@kyledurand kyledurand marked this pull request as ready for review October 21, 2020 23:30
@dfmcphee
Copy link
Contributor

I added a few comments to the web PR about this: https://github.com/Shopify/web/pull/33454.

I'm not opposed to it but I do worry a bit that it could lead to confusion.

@dfmcphee dfmcphee mentioned this pull request Oct 22, 2020
Merged
6 tasks
@kyledurand kyledurand closed this Oct 22, 2020
@kyledurand kyledurand reopened this Oct 22, 2020
@kyledurand
Copy link
Member Author

Meant to delete the branch but I hit reopen 🐒

@kyledurand kyledurand closed this Oct 22, 2020
@kyledurand kyledurand deleted the conditionally-render-custom-properties branch October 22, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alex-page alex-page alex-page approved these changes

@dfmcphee dfmcphee Awaiting requested review from dfmcphee

Assignees

No one assigned

Labels

🤖Skip Changelog Causes CI to ignore changelog update check.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kyledurand @dfmcphee @alex-page