Skip to content
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.

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

custom-properties: extend ThemeProvider to insert CSS Custom Props #982

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

iampava
Copy link

@iampava iampava commented Jun 5, 2020

  • Changed from default export to named exports
  • Added withCustomProperties to extend ThemeProvidr and insert the CSS Props in DOM.
  • Updated README

More info about the reasons here: #979.

@iampava iampava force-pushed the css-props-provider branch 3 times, most recently from b3c06c2 to d800b17 Compare June 5, 2020 13:21
@iampava
Copy link
Author

iampava commented Jun 5, 2020

Tests are faling. Checked out on master and ran yarn install && yarn test and still failing there.

@jxnblk
Copy link
Member

jxnblk commented Jun 5, 2020

Thanks! A few quick thoughts before I review this:

  1. We shouldn't remove the default export, since that would mean a breaking change.
  2. Why did you decide to use a higher order component? I would guess that we could handle this as a normal React component
  3. Tests aren't failing on master, so I'd guess it might be related to the changes you made in the lock file, if that helps unblock you

@iampava iampava force-pushed the css-props-provider branch 6 times, most recently from 56dfbf9 to 02de38c Compare June 6, 2020 12:47
@iampava
Copy link
Author

iampava commented Jun 6, 2020

Hey, thanks for the input.

  1. Reverted back to it. I was leaning towards a breaking change to keep the API consistent (all named exports) but this works as well. (PS: changed the version in package.json. Should I do this or not?)

  2. Added another feature'ish - the ability to select the class on the wrapper. I went with a HOC so that, if you have multiple providers inside the same app, you don't have to duplicate the prefix and className every time.

<CustomPropsThemeProvider theme={ subTheme } prefix="myApp" className="myApp-theme-provider" />

What do you think? It's not a huge copy-paste so I'd be fine with removing the HOC

  1. Yes, you're right. The tests weren't failing, the coverage was. Wrote some tests for the new function, but couldn't find a way to test that the CSS props have been declared on the element.

Tried all sorts of stuff, but every time calls like div.style.getPropertyValue('--color-primary') were empty string. I'm not too good on testing so if you've ever stumbled upon this, maybe you could provide some hints/docs.

@iampava iampava force-pushed the css-props-provider branch 5 times, most recently from aeb72dc to 3ac15c4 Compare June 7, 2020 08:38
@iampava
Copy link
Author

iampava commented Jun 28, 2020

Up!

@iampava
Copy link
Author

iampava commented Sep 27, 2020

Hey @jxnblk, @hasparus. Can we merge this?

@lachlanjc lachlanjc added the enhancement New feature or request label Dec 3, 2020
@lachlanjc lachlanjc linked an issue Dec 6, 2020 that may be closed by this pull request
Copy link
Collaborator

@atanasster atanasster left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the contribution

@hasparus hasparus closed this Dec 14, 2020
@hasparus hasparus deleted the branch system-ui:develop December 14, 2020 08:53
@hasparus hasparus reopened this Dec 14, 2020
@vercel
Copy link

vercel bot commented Dec 14, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/systemui/theme-ui/i5gkol31r
✅ Preview: Failed

@hasparus hasparus changed the base branch from master to develop December 14, 2020 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose theme values as CSS Variables
6 participants