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

FIX base theme initialization and theme bootup #5843

Merged
merged 20 commits into from
Mar 5, 2019

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Mar 4, 2019

Issue: #5824 #5780

creating a theme that derived off of light or dark 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.

@vercel
Copy link

vercel bot commented Mar 4, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@vercel vercel bot requested a deployment to staging March 4, 2019 11:11 Abandoned
@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #5843 into next will increase coverage by 0.69%.
The diff coverage is 65.75%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/theming/src/base.ts 100% <ø> (+100%) ⬆️
lib/ui/src/index.js 63.63% <0%> (ø) ⬆️
lib/theming/src/ensure.ts 0% <0%> (ø) ⬆️
lib/theming/src/themes/light.ts 100% <100%> (+100%) ⬆️
lib/theming/src/themes/dark.ts 100% <100%> (+100%) ⬆️
...i/src/components/sidebar/SidebarHeading.stories.js 100% <100%> (ø) ⬆️
lib/ui/src/core/layout.js 13.79% <5.88%> (-2.88%) ⬇️
lib/theming/src/create.ts 89.58% <92.85%> (+89.58%) ⬆️
lib/theming/src/animation.ts 100% <0%> (+100%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15312c1...52968c9. Read the comment docs.

Copy link
Member

@shilman shilman left a 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?

Copy link
Member

@tmeasday tmeasday left a 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

lib/theming/src/themes/dark.ts Outdated Show resolved Hide resolved
lib/theming/src/create.ts Outdated Show resolved Hide resolved
lib/theming/src/create.ts Outdated Show resolved Hide resolved
@shilman
Copy link
Member

shilman commented Mar 4, 2019

When I add this line to offcial-storybook/config.js the app hangs:

    theme: create({ base: 'dark', colorPrimary: '#FF4785', colorSecondary: '#1EA7FD' }),

If it helps, the console error is:

VM158:1 Uncaught TypeError: Cannot read property 'name' of undefined

@ndelangen
Copy link
Member Author

I'm writing tests @shilman

@vercel vercel bot requested a deployment to staging March 4, 2019 14:54 Abandoned
@shilman
Copy link
Member

shilman commented Mar 4, 2019

@ndelangen A couple comments, both outside the scope of this PR:

  • After testing out a buggy version, I had to do localStorage.clear() to get the theme to load. Seems like a bug, but maybe it's a corner case
  • Seems like the theme serialization should be profiled.. pretty slow to load on my laptop

Copy link
Member

@shilman shilman left a 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?

@vercel vercel bot requested a deployment to staging March 4, 2019 16:35 Abandoned
@ndelangen
Copy link
Member Author

@shilman

I'd like to remove the need to do theme serialization asap.

theme.color.primary seems to be very rarely used, @domyen came up with the naming and usage of colors.

@domyen
Copy link
Member

domyen commented Mar 4, 2019

@ndelangen yea theme.color.primary is not used at all, happy for you to axe it! (the var was mirrored from storybook/frontpage, but we don't need it here)

@ndelangen
Copy link
Member Author

I wrote in such a way, users won't need to change anything.
As long as they are using create()..

@ndelangen
Copy link
Member Author

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 deepEqual(theme, updatedTheme).

@vercel vercel bot requested a deployment to staging March 4, 2019 20:19 Abandoned
@vercel vercel bot requested a deployment to staging March 4, 2019 20:56 Abandoned
@vercel vercel bot requested a deployment to staging March 4, 2019 23:04 Abandoned
@vercel vercel bot requested a deployment to staging March 4, 2019 23:51 Abandoned
@vercel vercel bot requested a deployment to staging March 5, 2019 00:12 Abandoned
@vercel vercel bot requested a deployment to staging March 5, 2019 01:11 Abandoned
…tions-with-defaults

Tech/workaround persisted options with defaults
@vercel vercel bot requested a deployment to staging March 5, 2019 01:16 Abandoned
@shilman shilman changed the title IMPROVE theme creation FIX base theme initialization and theme bootup Mar 5, 2019
@shilman shilman merged commit 4878458 into next Mar 5, 2019
@shilman shilman deleted the tech/improve-theme-creating branch March 5, 2019 01:29
shilman added a commit that referenced this pull request Mar 5, 2019
FIX base theme initialization and theme bootup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants