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

[Addon-Knobs] Fixed addon for RN #1635

Merged
merged 3 commits into from
Aug 16, 2017
Merged

[Addon-Knobs] Fixed addon for RN #1635

merged 3 commits into from
Aug 16, 2017

Conversation

Gongreg
Copy link
Member

@Gongreg Gongreg commented Aug 10, 2017

Issue: #1586 Addon-knobs wasn't working on React Native

After cleanup done in 4c1daab addon wasn't breaking RN (since channel is not set while user is not connected to between app and browser).

What I did

Pretty much reverted the change back.

How to test

Adding knob examples to crna and react-vanilla-apps in a separate pr.

@codecov
Copy link

codecov bot commented Aug 11, 2017

Codecov Report

Merging #1635 into master will increase coverage by 0.14%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1635      +/-   ##
==========================================
+ Coverage   21.13%   21.27%   +0.14%     
==========================================
  Files         247      244       -3     
  Lines        5578     5386     -192     
  Branches      672      664       -8     
==========================================
- Hits         1179     1146      -33     
+ Misses       3879     3753     -126     
+ Partials      520      487      -33
Impacted Files Coverage Δ
addons/knobs/src/index.js 0% <ø> (ø) ⬆️
addons/knobs/src/KnobManager.js 32% <0%> (-3.42%) ⬇️
lib/ui/src/modules/api/actions/api.js 49.42% <0%> (-1.06%) ⬇️
.../src/manager/containers/CommentsPanel/dataStore.js 34.78% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/left_panel.js 25.71% <0%> (ø) ⬆️
addons/events/src/components/Event.js 0% <0%> (ø) ⬆️
addons/storyshots/src/require_context.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/types/Object.js 5.81% <0%> (ø) ⬆️
addons/info/src/components/Story.js 76.19% <0%> (ø) ⬆️
addons/info/src/components/Node.js 38.88% <0%> (ø) ⬆️
... and 21 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 39c9a65...dbe2479. Read the comment docs.

@danielduan
Copy link
Member

is there anything preventing us from merging this? would love to get this into the next patch release.

@ndelangen
Copy link
Member

Code LGTM, maybe @tmeasday wants to veto this and merge?

@tmeasday
Copy link
Member

I don't really understand @Gongreg's comment.

Are you saying that it was broken after the refactor and this fixes it? And what do you mean about the channel? (sorry too many negatives in the sentence, I can't parse it ;) )

@Gongreg
Copy link
Member Author

Gongreg commented Aug 16, 2017

@tmeasday, Yes, I mean it broke after refactor and this commit fixes it.

By channel I mean that in RN app the channel is not instantly set. See this code:

app/react-native/src/preview/index.js

getStorybookUI(params = {}) {
    return () => {
      let channel = addons.getChannel();
      if (params.resetStorybook || !channel) {
        ...
        channel = createChannel({ url });
        addons.setChannel(channel);
      }
      ...
  }

And after refactor the code addons.getChannel() existed in global scope so it was required before channel was set. It is difficult to explain, but we have to require channel only before sending event in it, not at the start of the program.

@tmeasday
Copy link
Member

Thanks for that @Gongreg, yes it makes perfect sense to me (cf #1191)! I just was making sure I understood what you were saying correctly.

I haven't had a chance to test this but the code changes make sense to me.

@Gongreg
Copy link
Member Author

Gongreg commented Aug 16, 2017

I also added examples in #1636 for RN, so at least we could see in future if it stops working. But the pr checks screams to me about code duplication. I just have other priorities atm to fix it.

@ndelangen
Copy link
Member

Either of you want to merge this then? @Gongreg @tmeasday

@tmeasday tmeasday merged commit d486e17 into master Aug 16, 2017
@ndelangen ndelangen deleted the addon-knobs-rn-fix branch August 16, 2017 11:46
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