FIX theming bugs #5787

ndelangen · 2019-02-27T23:21:56Z

What I did

FIX the theme not showing on first load
persist the theme into localstorage to prevent flash upon reload

I fixed this a bit quick and dirty, Will cleanup after V5 release

…nt flashing upon reloading

vercel · 2019-02-27T23:22:01Z

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

tmeasday

LGTM. Agreed that further refactoring into core/ui etc probably makes sense

# Conflicts: # examples/official-storybook/tests/__snapshots__/storyshots.test.js.snap

…e theme was getting lost ADD telejson as stringifier & parser for persistence CLEANUP state initialization CHANGE initial state for shortcuts so it's not dependent on this.getState() DEDUPLICATE merge util function FIX unnecessary messages about transition & brand missing in theme

codecov · 2019-02-28T21:56:03Z

Codecov Report

Merging #5787 into next will decrease coverage by 0.02%.
The diff coverage is 23.8%.

@@           Coverage Diff            @@
##           next    #5787      +/-   ##
========================================
- Coverage    34%   33.97%   -0.03%     
========================================
  Files       651      651              
  Lines      9473     9465       -8     
  Branches   1345     1368      +23     
========================================
- Hits       3221     3216       -5     
+ Misses     5637     5635       -2     
+ Partials    615      614       -1

Impacted Files	Coverage Δ
lib/ui/src/core/initial-state.js	`100% <ø> (ø)`	⬆️
lib/ui/src/core/init-provider-api.js	`0% <ø> (-25.81%)`	⬇️
lib/ui/src/core/stories.js	`87.23% <ø> (+0.44%)`	⬆️
lib/ui/src/core/context.js	`9.09% <0%> (ø)`	⬆️
lib/ui/src/core/addons.js	`0% <0%> (ø)`	⬆️
lib/channel-postmessage/src/index.ts	`0% <0%> (ø)`	⬆️
lib/ui/src/components/layout/container.js	`20.37% <0%> (ø)`	⬆️
lib/theming/src/ensure.ts	`0% <0%> (ø)`	⬆️
lib/ui/src/core/store.js	`100% <100%> (ø)`	⬆️
lib/ui/src/core/layout.js	`16.66% <25%> (+16.66%)`	⬆️
... and 1 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 5d60575...6489d05. Read the comment docs.

ndelangen · 2019-02-28T22:22:10Z

@tmeasday Ended up having to introduce telejson in the persistence anyway, and had to up the maxDepth a bit for postMessage (not the entire theme was getting through)

tmeasday

We decided not to persistence this way.. open to changing it in the future but not at this stage of the release.

lib/ui/src/core/context.js

lib/ui/src/core/store.js

tmeasday · 2019-03-01T02:52:56Z

Ok with the rest of it, but if the issue was combining the theme defaults with the persisted theme, that should just be done the same way that e.g. versions does it.

…inimal panelsize & maximum main size)

REMOVE merge from initial state MOVE initial state of layout & ui to layout module ADD merge of initial, restored to layout module

lib/ui/src/core/init-provider-api.js

ndelangen · 2019-03-01T15:24:50Z

@tmeasday thanks for the comments, You're right, I changed it back.

tmeasday

I think this is OK but i have a couple of comments that we should probably discuss and resolve in future versions

tmeasday · 2019-03-02T01:10:04Z

lib/ui/src/core/addons.js

+      selectedPanel: ensurePanel(
+        api.getPanels(),
+        store.getState().selectedPanel,
+        store.getState().selectedPanel


This is a bit weerd (grabbing the selected panel twice). Is it intentional?

Yeah, what's going on here?

Just followed the arguments of ensurePanel,
I can remove 1 I think.

tmeasday · 2019-03-02T01:12:43Z

lib/ui/src/core/layout.js

+            selectedPanel: options.panel || options.selectedPanel || selectedPanel,
+          },
+          { persistence: 'permanent' }
+        );


Does this mean we perma-persist all options, including the ones that were session-persisted above?

I'm OK with this for now for simplicity, but we might want to do something more subtle in the future (which will probably require improvements to the persistence API)

Yeah I briefly experimented with persisting all the layout & ui options, but decided to not increase the size of this PR even more.

But it was actually really nice, things like full-screen, nav, panelPosition being preserved over refreshes...

On my todo list after V5 launch.

lib/ui/src/core/shortcuts.js

tmeasday · 2019-03-02T01:15:05Z

Thanks for pulling the layout module out @ndelangen !

FIX theming bugs

FIX the themeing flash & persist the theme into localstorage to preve…

362a339

…nt flashing upon reloading

ndelangen added bug theming release labels Feb 27, 2019

ndelangen added this to the v5.0.0 milestone Feb 27, 2019

ndelangen self-assigned this Feb 27, 2019

ndelangen requested review from danielduan, Hypnosphi, igor-dv, tmeasday and usulpro as code owners February 27, 2019 23:21

tmeasday approved these changes Feb 28, 2019

View reviewed changes

Merge branch 'next' into fix/theming-flash

239d9f3

# Conflicts: # examples/official-storybook/tests/__snapshots__/storyshots.test.js.snap

vercel bot requested a deployment to staging February 28, 2019 12:52 Abandoned

ndelangen added 3 commits February 28, 2019 14:31

REMOVE options from snapshot

2eed5f7

FIX problem with loading empty localStorage to theme

9a5ccca

ndelangen requested a review from thani-sh as a code owner February 28, 2019 21:56

vercel bot temporarily deployed to staging February 28, 2019 21:56 Inactive

REMOVE dark theme

ef1f79c

vercel bot had a problem deploying to staging February 28, 2019 21:58 Failure

FIX typescript issue

d9bb177

vercel bot requested a deployment to staging February 28, 2019 22:17 Abandoned

tmeasday requested changes Mar 1, 2019

View reviewed changes

lib/ui/src/core/context.js Show resolved Hide resolved

lib/ui/src/core/store.js Outdated Show resolved Hide resolved

IMPROVE the default size of panels when switching bottom and right (m…

16ff723

…inimal panelsize & maximum main size)

vercel bot deployed to staging March 1, 2019 11:52 View deployment

vercel bot requested a deployment to staging March 1, 2019 14:59 Abandoned

ndelangen force-pushed the fix/theming-flash branch from 180ca78 to 9c5c780 Compare March 1, 2019 15:21

vercel bot requested a deployment to staging March 1, 2019 15:21 Abandoned

MOVE setOptions to layout module

7622f72

REMOVE merge from initial state MOVE initial state of layout & ui to layout module ADD merge of initial, restored to layout module

ndelangen force-pushed the fix/theming-flash branch from 9c5c780 to 7622f72 Compare March 1, 2019 15:22

vercel bot requested a deployment to staging March 1, 2019 15:22 Abandoned

ndelangen commented Mar 1, 2019

View reviewed changes

lib/ui/src/core/init-provider-api.js Show resolved Hide resolved

FIX theming bug

f03d7e9

ndelangen mentioned this pull request Mar 1, 2019

Using any theme makes addon-a11y crash #5780

Closed

vercel bot requested a deployment to staging March 1, 2019 16:10 Abandoned

FIX selectedPanel not being stored to persisted store

81770cc

vercel bot requested a deployment to staging March 1, 2019 16:14 Abandoned

tmeasday approved these changes Mar 2, 2019

View reviewed changes

ndelangen added 2 commits March 2, 2019 18:08

Merge branch 'next' into fix/theming-flash

242e611

CLEANUP

6489d05

vercel bot deployed to staging March 2, 2019 17:25 View deployment

ndelangen merged commit b9cce09 into next Mar 2, 2019

ndelangen deleted the fix/theming-flash branch March 2, 2019 18:13

shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 3, 2019

shilman pushed a commit that referenced this pull request Mar 3, 2019

Merge pull request #5787 from storybooks/fix/theming-flash

694f8e9

FIX theming bugs

shilman mentioned this pull request Mar 3, 2019

Custom theme is only applied after navigating to a story #5746

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX theming bugs #5787

FIX theming bugs #5787

ndelangen commented Feb 27, 2019

vercel bot commented Feb 27, 2019

tmeasday left a comment

codecov bot commented Feb 28, 2019 •

edited

Loading

ndelangen commented Feb 28, 2019

tmeasday left a comment

tmeasday commented Mar 1, 2019

ndelangen commented Mar 1, 2019

tmeasday left a comment

tmeasday Mar 2, 2019

shilman Mar 2, 2019

ndelangen Mar 2, 2019

tmeasday Mar 2, 2019

ndelangen Mar 2, 2019

tmeasday commented Mar 2, 2019

FIX theming bugs #5787

FIX theming bugs #5787

Conversation

ndelangen commented Feb 27, 2019

What I did

vercel bot commented Feb 27, 2019

tmeasday left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 28, 2019 • edited Loading

Codecov Report

ndelangen commented Feb 28, 2019

tmeasday left a comment

Choose a reason for hiding this comment

tmeasday commented Mar 1, 2019

ndelangen commented Mar 1, 2019

tmeasday left a comment

Choose a reason for hiding this comment

tmeasday Mar 2, 2019

Choose a reason for hiding this comment

shilman Mar 2, 2019

Choose a reason for hiding this comment

ndelangen Mar 2, 2019

Choose a reason for hiding this comment

tmeasday Mar 2, 2019

Choose a reason for hiding this comment

ndelangen Mar 2, 2019

Choose a reason for hiding this comment

tmeasday commented Mar 2, 2019

codecov bot commented Feb 28, 2019 •

edited

Loading