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

[Hosted RN Storybook] Added addon for pairing device (with qr code). #1639

Closed
wants to merge 15 commits into from

Conversation

Gongreg
Copy link
Member

@Gongreg Gongreg commented Aug 11, 2017

Issue: If user has hosted react native storybook version, it needs to manually enter code to the phone to connect it to server.

This addon solves the issue by displaying qr code, so user can simply point the phone to it to pair.

What I did

Added new event when new channel is created. This addon listens to it and uses the pairedId code for qr component.

Example

https://expo.io/@gongreg/rn-storybook-hosted-client-example

Server: https://vast-eyrie-45947.herokuapp.com/ (takes ~30s to load because heroku shuts it down).


This change is Reviewable

@Gongreg Gongreg changed the title Added addon for pairing device with hosted rn storybook (with qr code). [Hosted RN Storybook] Added addon for pairing device (with qr code). Aug 11, 2017
@codecov
Copy link

codecov bot commented Aug 17, 2017

Codecov Report

Merging #1639 into master will decrease coverage by 71.31%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1639       +/-   ##
===========================================
- Coverage    92.5%   21.18%   -71.32%     
===========================================
  Files           6      263      +257     
  Lines          40     5777     +5737     
  Branches        2      689      +687     
===========================================
+ Hits           37     1224     +1187     
- Misses          2     4029     +4027     
- Partials        1      524      +523
Impacted Files Coverage Δ
addons/rn-pair/register.js 0% <0%> (ø)
addons/rn-pair/src/containers/QRCodePanel/index.js 0% <0%> (ø)
app/react-native/src/manager/provider.js 0% <0%> (ø)
addons/rn-pair/src/manager.js 0% <0%> (ø)
addons/rn-pair/src/components/QRCode/style.js 0% <0%> (ø)
addons/rn-pair/src/index.js 0% <0%> (ø)
addons/rn-pair/src/components/QRCode/index.js 0% <0%> (ø)
...tive-vanilla/storybook/stories/Button/index.ios.js
...tive-vanilla/storybook/stories/CenterView/index.js
...-native-vanilla/storybook/stories/Welcome/index.js
... and 266 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 34b98a8...29fe2a4. Read the comment docs.

@ndelangen
Copy link
Member

@Gongreg is this still something you're working on?

@Gongreg
Copy link
Member Author

Gongreg commented Sep 6, 2017

@ndelangen, yes. I am working with it internally, trying it out, before moving forward here.

@Gongreg
Copy link
Member Author

Gongreg commented Dec 18, 2017

Hey, I still want to do some cleanup. But here is an example project: https://expo.io/@gongreg/rn-storybook-hosted-client-example

Server: https://vast-eyrie-45947.herokuapp.com/ (takes ~30s to load because heroku shuts it down).

@ndelangen
Copy link
Member

@Gongreg What's the next step for this? After 3.3 is merged and released, what's you plan?

@@ -0,0 +1,28 @@
# Storybook Addon Actions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Title is incorrect


Then, add following content to `.storybook/addons.js`

import '@storybook/addon-rn-pair/register';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide a fenced code format with a language

@@ -0,0 +1,37 @@
{
"name": "@storybook/rn-pair",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is an addon, it's package should be named accordingly: @storybook/addon-<name>

I'd say maybe QR-Pair is a better name for this addon, what do you think?
@storybook/addon-qrpair

"scripts": {
"deploy-storybook": "storybook-to-ghpages",
"prepublish": "node ../../scripts/prepublish.js",
"storybook": "start-storybook -p 9001"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not include a 'embedded' storybook example. Place storybook examples, in the <root>/examples/.

@@ -0,0 +1,8 @@
export default {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use glamorous for styling components.

@AhmadEl-Banna
Copy link

you can use now for publishing instead of Heroku, it's much better

@Hypnosphi
Copy link
Member

@ndelangen is there anything else to do here?

@Hypnosphi Hypnosphi added this to the v4.0.0 milestone Mar 8, 2018
@Gongreg
Copy link
Member Author

Gongreg commented Mar 24, 2018

Hey, I've thought about this for quite some time. The more I think about it the more I realise that there is almost no need for this functionality.
This functionality doesn't really bring anything for the developer. He will still use locally running storybook server + app.
If we are talking about user who is just playing around in a storybook, why would he want to open both app and browser? First question is going to be, why do I even need browser? What we should focus on is making onDeviceUI support a lot better. It should have option to display all the addons (that are possible) user wants to use. It should have a proper hierarchy of stories just like there is one in browser. I know that I spent lots of time trying to make it work, but I think it is better to not clutter storybook more with functionality, that is not really going to be used.
I've tried to write a medium post explaining what I did here and I showed it for few people, but they were not really sure why do we need to do this all complex thing.

So instead I am going to close this PR and update the medium blogpost into something more of "Trying to create static React Native Storybook and steps forward" where I will share experience in creating this and look into what we can actually achieve with onDeviceUI instead of this.

Sorry that I kept this open for so long, but I will try to be more useful while working onDeviceUI part.

@ndelangen ndelangen closed this Mar 26, 2018
@ndelangen
Copy link
Member

@Gongreg I imagine a feature that shows a full screen on-device navigator (with hierarchy) when you some gesture or hold the phone a certain way.

I do think the web UI makes sense for documentation purposes also for RN. And i also would ask how to make use of things like addon-knobs for RN if there's no web UI connected to it.

Hoping to see some PR's improving the on-device navigator from you ;)

@Gongreg Gongreg deleted the addon-rn-pair branch October 10, 2018 06:57
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