-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
FIX base theme initialization and theme bootup #5843
Conversation
…ll merge in the appropriate themeVars
This pull request is automatically deployed with Now. |
Codecov Report
@@ Coverage Diff @@
## next #5843 +/- ##
=========================================
+ Coverage 34.3% 34.99% +0.69%
=========================================
Files 648 648
Lines 9465 9478 +13
Branches 1344 1332 -12
=========================================
+ Hits 3247 3317 +70
+ Misses 5600 5533 -67
- Partials 618 628 +10
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndelangen please test the hell out of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this code would benefit from some tests but I appreciate this is a fairly late change
When I add this line to
If it helps, the console error is:
|
I'm writing tests @shilman |
…erties && FIX merge order of vars & defaults
@ndelangen A couple comments, both outside the scope of this PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndelangen Maybe a stupid question:
theme: create({ base: 'dark', colorPrimary: 'red', colorSecondary: 'green' }),
I did this in official-storybook
. I see green in the resulting UI, but I don't see any red. Doesn't seem right to me?
@ndelangen yea |
I wrote in such a way, users won't need to change anything. |
I also noticed that setOptions is called a ton, and it's was re-creating the theme on every navigation. It'd be nice to fix this a bit better, but for now I improved it by |
…lorSecondary` as fallback
Ensure the theme is insta-loaded from local storage
…tions-with-defaults Tech/workaround persisted options with defaults
FIX base theme initialization and theme bootup
Issue: #5824 #5780
creating a theme that derived off of
light
ordark
default themes was impractical, and wasn't working quite as users were expecting it to behave.What I did
ADD ability for users to specify baseTheme, and the theme createFn will merge in the appropriate themeVars.