Skip to content

[ThemeProvider][Color system] clean up theme provider in preparation for color system work #2225

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 3 commits into from
Oct 3, 2019
Merged

[ThemeProvider][Color system] clean up theme provider in preparation for color system work #2225

merged 3 commits into from
Oct 3, 2019

Conversation

@danrosenthal
Copy link

@danrosenthal danrosenthal commented Oct 3, 2019 •
edited
Loading

Co-authored-by: Tim Layton tim.layton@shopify.com
Co-authored-by: Ben Scott ben.scott@shopify.com

Depends on #2224

  • 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)

@danrosenthal danrosenthal force-pushed the theme-provider-cleanup branch from 359d0e4 to 6b50dd2 Compare October 3, 2019 12:49
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2019 •
edited
Loading

Results

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified13
Files potentially affected33

Details

All files potentially affected (total: 33)
❔ UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/AppProvider/AppProvider.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/PolarisTestProvider/PolarisTestProvider.tsx (total: 0)

Files potentially affected (total: 0)

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

Files potentially affected (total: 1)

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

Files potentially affected (total: 0)

🧩 src/utilities/color-manipulation.ts (total: 29)

Files potentially affected (total: 29)

🧩 src/utilities/color-transformers.ts (total: 31)

Files potentially affected (total: 31)

🧩 src/utilities/tests/color-manipulation.test.ts (total: 0)

Files potentially affected (total: 0)

🧩 src/utilities/tests/color-transformers.test.ts (total: 0)

Files potentially affected (total: 0)

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

Files potentially affected (total: 27)

🧩 src/utilities/theme/tests/utils.test.ts (total: 0)

Files potentially affected (total: 0)

🧩 src/utilities/theme/types.ts (total: 31)

Files potentially affected (total: 31)

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

Files potentially affected (total: 28)

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

@danrosenthal danrosenthal force-pushed the theme-provider-cleanup branch from 6b50dd2 to a4d9d52 Compare October 3, 2019 13:09
@danrosenthal danrosenthal marked this pull request as ready for review October 3, 2019 13:26
@danrosenthal danrosenthal force-pushed the theme-provider-cleanup branch from 5c10762 to bcd24c4 Compare October 3, 2019 14:43
@danrosenthal danrosenthal changed the base branch from master to portal-within-theme-provider-node October 3, 2019 14:43
@danrosenthal danrosenthal force-pushed the portal-within-theme-provider-node branch from 5c1124f to abf05db Compare October 3, 2019 14:47
@danrosenthal danrosenthal force-pushed the theme-provider-cleanup branch 2 times, most recently from 7a8cfcf to 3f2522d Compare October 3, 2019 14:51
@danrosenthal danrosenthal force-pushed the portal-within-theme-provider-node branch 2 times, most recently from 4d076ca to d62cff6 Compare October 3, 2019 17:55
danrosenthal and others added 2 commits October 3, 2019 13:57
@danrosenthal @BPScott
clean up theme provider in preparation for color system work
83d1ae4 
Co-authored-by: Tim Layton <tim.layton@shopify.com>
Co-authored-by: Ben Scott <ben.scott@shopify.com>
@danrosenthal
a bit more tidying
1a59690 
Co-authored-by: Tim Layton <tim.layton@shopify.com>
@danrosenthal danrosenthal force-pushed the theme-provider-cleanup branch from 0f9424a to 1a59690 Compare October 3, 2019 17:59
tmlayton
tmlayton approved these changes Oct 3, 2019
View reviewed changes
Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

:shipit:

@danrosenthal @tmlayton
Update UNRELEASED.md
f510018 
Co-Authored-By: Tim Layton <tmlayton@users.noreply.github.com>
@danrosenthal
Copy link
Author

danrosenthal commented Oct 3, 2019

Code coverage

Ran code coverage reports locally and they look good.

AppProvider PolarisTestProvider ThemeProvider theme/utils
screencapture-file-Users-danrosenthal-Projects-polaris-react-build-coverage-src-components-AppProvider-AppProvider-tsx-html-2019-10-03-14_33_00 screencapture-file-Users-danrosenthal-Projects-polaris-react-build-coverage-src-components-PolarisTestProvider-PolarisTestProvider-tsx-html-2019-10-03-14_31_21 screencapture-file-Users-danrosenthal-Projects-polaris-react-build-coverage-src-components-ThemeProvider-ThemeProvider-tsx-html-2019-10-03-14_31_33 screencapture-file-Users-danrosenthal-Projects-polaris-react-build-coverage-src-utilities-theme-utils-ts-html-2019-10-03-14_31_43

The check didn't actually run in CI, and failed previously.

@danrosenthal danrosenthal merged commit ccba5aa into portal-within-theme-provider-node Oct 3, 2019
@tmlayton tmlayton deleted the theme-provider-cleanup branch October 3, 2019 18:48
@danrosenthal danrosenthal restored the theme-provider-cleanup branch October 3, 2019 19:04
This was referenced Oct 3, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AndrewMusgrave AndrewMusgrave Awaiting requested review from AndrewMusgrave

@BPScott BPScott Awaiting requested review from BPScott

1 more reviewer

@tmlayton tmlayton tmlayton approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@danrosenthal @tmlayton