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

Merge master into shilman/refactor-core #2628

Merged
merged 13 commits into from
Jan 3, 2018

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Jan 3, 2018

Issue: #2241 was out of date

What I did

Merged master. Also re-applied #2349 in ad97f32

How to test

Please test all 3 web-based frameworks, especially around HMR and the issues explored in #2349

shilman and others added 10 commits November 6, 2017 00:52
Questionable changes:
- ability to pass a story decorator function
- pass clearDecorators to ConfigAPI

React/Vue/Angular working (apparently). RN still broken.
- resolve conflicts
- fix RN story_store (make it an EventEmitter)
- move keyboard handling to the UI library, rather than introducing new
deps
- keep QS handling in each framework (for now)
…factor

Use store revisions to ensure that stories re-render on HMR.
@tmeasday tmeasday requested review from shilman and a team January 3, 2018 06:56
@tmeasday
Copy link
Member Author

tmeasday commented Jan 3, 2018

We could also just merge this directly into master, if that is preferred.

@igor-dv
Copy link
Member

igor-dv commented Jan 3, 2018

Oh, man 🤕 ...

@tmeasday
Copy link
Member Author

tmeasday commented Jan 3, 2018

I'm not sure how best to show the diff between this branch and @shilman's? Perhaps I should just change the base of this to master?

This didn't apply at all cleanly as it involved adding code to each framework's `init.js`, which was removed in the refactor.

Here we do better; we simply abstract the job of URL<->redux store syncing to the core library.
@tmeasday tmeasday force-pushed the tmeasday/shilman-refactor-core-rebase branch from a4530fe to ad97f32 Compare January 3, 2018 10:07
@codecov
Copy link

codecov bot commented Jan 3, 2018

Codecov Report

Merging #2628 into master will increase coverage by 1.68%.
The diff coverage is 31.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2628      +/-   ##
==========================================
+ Coverage   32.65%   34.33%   +1.68%     
==========================================
  Files         398      388      -10     
  Lines        8868     8681     -187     
  Branches      968      913      -55     
==========================================
+ Hits         2896     2981      +85     
+ Misses       5298     5074     -224     
+ Partials      674      626      -48
Impacted Files Coverage Δ
lib/core/src/client/preview/reducer.js 37.5% <ø> (ø)
lib/core/src/client/preview/actions.js 40% <ø> (ø)
app/angular/src/client/preview/index.js 0% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 33.96% <0%> (-6.04%) ⬇️
app/vue/src/client/preview/index.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/index.js 0% <0%> (ø) ⬆️
lib/core/src/client/preview/index.js 0% <0%> (ø)
lib/core/src/client/index.js 0% <0%> (ø)
app/react-native/src/preview/index.js 0% <0%> (ø) ⬆️
lib/core/src/client/preview/client_api.js 91.48% <100%> (ø)
... and 65 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 1af4e8d...b60ac0a. Read the comment docs.

@igor-dv
Copy link
Member

igor-dv commented Jan 3, 2018

I think better merge it to #2241 and we will review it again there

@ndelangen
Copy link
Member

/tmp/storybook/app/angular/src/client/index.js
  1:56  error  getStorybook not found in './preview'  import/named

/tmp/storybook/app/vue/src/client/index.js
  7:56  error  getStorybook not found in './preview'  import/named

@tmeasday
Copy link
Member Author

tmeasday commented Jan 3, 2018

oops, sorry fixed this but forgot to push last night.

@tmeasday
Copy link
Member Author

tmeasday commented Jan 3, 2018

I think everyone is sufficiently weirded out by this PR, I will change the base to master.

@tmeasday tmeasday changed the base branch from shilman/refactor-core to master January 3, 2018 21:23
@tmeasday
Copy link
Member Author

tmeasday commented Jan 3, 2018

Hopefully a little easier to understand now.

@tmeasday
Copy link
Member Author

tmeasday commented Jan 3, 2018

This is passing now.

@ndelangen ndelangen merged commit b4dec5f into master Jan 3, 2018
@ndelangen ndelangen deleted the tmeasday/shilman-refactor-core-rebase branch January 3, 2018 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants