React Native - On Device UI #1413
Conversation
|
@matt-oakes this is awesome. I believe @ajwhite has also been working on something similar, so I hope you can coordinate your efforts. I'm snowed under for a few days but happy to join in once things clear up on my side. |
|
Love this idea! I think the efforts made here and the efforts I'm making on in-web simulators (#1125) can coexist wonderfully. My only suggestion would be to consider having a "docking" toggle since we have limited screen real estate on mobile! |
|
@shilman Thanks! @ajwhite I did see that as well. I like the idea of running a simulator in the preview window. If you are using appetize.io as suggested then I imagine running the UI in the simulator might be better for development, whereas running the simulator in the browser would be better for a component library website. Does that sound correct? I'll look into adding a toggle to switch between the two modes as well. |
|
Exactly @matt-oakes 😃
To clarify -- my thinking here was that this toggle would just collapse/expand the story navigation on the device UI (versus toggling between web simulator and device simulator). I think collapse/expanding is what you are referring to, but just wanted to clarify! |
| @@ -21,6 +24,8 @@ export default class StoryStore { | |||
| index: count, | |||
| fn, | |||
| }; | |||
|
|
|||
| this.emit('storyAdded', kind, name, fn); | |||
There was a problem hiding this comment.
Not sure how removeStory works/becomes called, but do we want to add an emitter for that as well for our StoryListView to subscribe to?
There was a problem hiding this comment.
I hadn't spotted the remove method. I can't think of a situation when that would be used...
There was a problem hiding this comment.
Neither can I - I'm not familiar enough with that part; cc @ndelangen
|
@ajwhite Yes! That's what I was thinking as well! Currently, I'm leaning towards creating a new module for this (something like |
matt-oakes
force-pushed
the
storybook-native-ui
branch
from
2fe3e8c to
44cfb53
Compare
Codecov Report
@@ Coverage Diff @@
## release/3.2 #1413 +/- ##
===============================================
- Coverage 20.55% 20.27% -0.29%
===============================================
Files 237 240 +3
Lines 5185 5188 +3
Branches 762 631 -131
===============================================
- Hits 1066 1052 -14
- Misses 3551 3668 +117
+ Partials 568 468 -100
Continue to review full report at Codecov.
|
|
I've worked on this a tiny bit more.
|
| return ( | ||
| <View style={style.main}> | ||
| <StoryListView stories={stories} events={events} /> | ||
| <StoryView url={url} events={events} /> | ||
| <View style={style.preview}> | ||
| <SegmentedControlIOS |
There was a problem hiding this comment.
Mind updating the PR gif with these latest changes?
There was a problem hiding this comment.
I've just added a screenshot, but I'll add a gif when it's a bit more thought through.
|
@matt-oakes love where you're going with this. somewhat related: #1223 |
|
I've now added a bit of styling to the panel and added an indicator to show which story is currently selected. For now, I have also removed the size picker for now as it can currently show the wrong size components due to the different densities of the devices. Here's what the panel currently looks like: As previously mentioned, this is enabled by a param on the I'm not too sure how far to take this without people beginning to use it as I'm not sure what features will be needed. I'd welcome any feedback on what I have so far! |
|
@matt-oakes @ajwhite we can potentially include this in 3.2-alpha for feedback if it's ready to merge. https://gist.github.com/shilman/947a3d1d4cfdf5c3a8bb06d3d4eb84cf I need to review the feature, and probably wrangle a few more reviews from maintainers before I add it, but that would be a way to gather some feedback from users. What do you think? |
@matt-oakes I think treating this as an MVP is a great way to introduce this and see how it's used and the feedback we receive. What you have now seems perfect, and we can continue to iterate on this base! |
| <SectionList | ||
| style={style.list} | ||
| renderItem={({ item }) => <ListItem title={item.name} selected={item.kind === this.state.selectedKind && item.name === this.state.selectedStory} onPress={() => this.changeStory(item.kind, item.name)} />} | ||
| renderSectionHeader={({ section }) => <SectionHeader title={section.title} selected={section.title === this.state.selectedKind} />} |
There was a problem hiding this comment.
Just some minor style nits - these lines are pretty long, mind breaking it up more like so?
renderItem={({ item}) => (
<ListItem
title={item.name}
selected={..}
...
/>
)}
...Or even having renderItem and renderSectionHeader as methods on the class
There was a problem hiding this comment.
Good spot. Fixed!
| @@ -25,7 +25,7 @@ | |||
| 2D02E4BC1E0B4A80006451C7 /* AppDelegate.m in Sources */ = {isa = PBXBuildFile; fileRef = 13B07FB01A68108700A75B9A /* AppDelegate.m */; }; | |||
| 2D02E4BD1E0B4A84006451C7 /* Images.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = 13B07FB51A68108700A75B9A /* Images.xcassets */; }; | |||
| 2D02E4BF1E0B4AB3006451C7 /* main.m in Sources */ = {isa = PBXBuildFile; fileRef = 13B07FB71A68108700A75B9A /* main.m */; }; | |||
| 2D02E4C21E0B4AEC006451C7 /* libRCTAnimation-tvOS.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 5E9157351DD0AC6500FF2AA8 /* libRCTAnimation-tvOS.a */; }; | |||
There was a problem hiding this comment.
I know we probably don't need this right now, but since all the other -tvOS libs are included, we should keep this so that one day if they're needed, there's no confusion why one was missing.
There was a problem hiding this comment.
This is a change which Xcode made when I changed the example to be universal rather than iPhone only. Looking at it there are two targets, one for the phone and another for tvOS. The tvOS one includes libRCTAnimation.a but there is no such framework as libRCTAnimation-tvOS.a listed in Xcode. I'm guessing that this was a change in React Native and the example project was using out of date references (at a complete guess).
I think for now we should go with what Xcode has changed it to to avoid commit noise in the future. It can always be fixed if there is an issue if/when tvOS support is added to Storybook.
There was a problem hiding this comment.
Thanks for the explanation! Sounds good 👍
|
@shilman That makes sense to me. It would be good to know how the interacts with the new story hierarchy. I've not tested it with that, but I assume it would just show the kind as "the/path/to"? @ajwhite Sounds great! Happy to help out when we start to get some feedback! I'm due to begin a new React Native app next week where we will likely be using Storybook, so I'll see how useful this feature is then. |
|
@matt-oakes great. i'm going to change the base branch to |
|
A heads up that there's a few merge conflicts, and if you're ready to take this out of WIP, feel free to update the title 😃 |
matt-oakes
force-pushed
the
storybook-native-ui
branch
from
4fc31fb to
3ad29c7
Compare
This is a combination of 10 commits. - Add a basic implementation of the native UI for the storybook - Fix the lint errors - Add a param to turn the native UI on and off using the same package - Correctly set the width of the story list - Added a size selector for the different sized iPhone screens - Show the selected story in bold - Add some styling to the panels - Removed the canvas size switcher - Fix lint errors - Shortened the long render item and header lines
matt-oakes
force-pushed
the
storybook-native-ui
branch
from
3ad29c7 to
86679eb
Compare
|
I think this is a great idea. It could eventually replace the web UI entirely, which simplifies RN usage. I had also wanted to move to using an iPad to be able to emulate different sizes easily, so I think this PR in general is a really great step forwards. I'd love to see it in. I like the idea of having it as a separate module today ( this to me is a major semver change if it replaces the web UI) One request is to be able to hide that sidebar as an option when you're creating the root component, I would continue to use vscode-react-native-storybooks for story selection, less mouse work. |
matt-oakes
changed the title
|
@ajwhite @shilman Thanks. After a hairy moment with @orta Thanks for the feedback! For now, it's implemented as a switch in the existing module to avoid code duplication, but there's no reason it needs to stay this way in the future. Nice work with the VSCode panel by the way. I might have to check that out as an alternative! |
There was a problem hiding this comment.
@matt-oakes got this running on my machine and it generally looks great. We'll def want to update the docs before release, but I think it's good enough to alpha.
My one request. If onDeviceUI is enabled, we should automatically select the first story when it loads, instead of displaying this message:
|
This looks great! I think we should have the discussion that @matt-oakes mentioned in his first comment. Namely, how do we expect addons/code-sharing to work if we have two different UI code bases? My 2c: I'd say the best approach would be to look at this "native" UI as a very stripped down, minimal version of storybook. So there's no expectation that any UI-driven addon would work or that new features like nested chapters would be supported. One advantage of taking that approach would be that we could do a totally different UI for such a version. My thought for RN was always that it would be cool to hook into RN's developer menu somehow. If we could get a simple list of stories in there I think it would be very natural. |
|
Hey what's the plan for getting this merged? |
|
I can make the change that @shilman requested, but I won't be able to do it for a few weeks as I've just begun a new project. Either I can get to it then or someone else can pick up that remaining task so this can be merged in if you would prefer it to be done sooner. |
|
@matt-oakes thanks, I'm going to merge this then and will make the requested change myself. Opened a separate issue here: #1492 |
|
@shilman Sounds like a good plan to me! |
What I did
I created a prototype of an idea I had while last working with Storybook on a React Native project. As it stands, you currently need to have the simulator/emulator open to preview each scene and the browser open for the main UI. This has a few issues:
To solve this issue, I propose moving the story picker (and potentially the addon panels) into the simulator. You would then be able to run Storybook inside an iPad or Android Tablet simulator and not have to worry about the browser UI.
Prototype Implementation
I don't expect this pull request to be merged in, it is intended to be the beginning of a discussion about the feature.
In this implementation, I have modified the existing React Native preview code to include a
StoryListViewthis is linked together to thePreviewclass using the existingEventEmitterand store. I have made some small modifications to the events to wire everything together, but this is a very rough and ready solution (see below for the considerations I can think of).These changes end up looking like this with an iPhone simulator in landscape:
(The slowness is the gif, not the implementation)
The idea is to expand this so the preview area on the right can be sized and the three different iPhone sizes and orientations for testing components on the different sized screen. With the resizable simulator, it might even be possible to include the iPad resolutions using a super-high-res simulator (I'm not certain that that is possible).
Discussions which need to happen
I think this is a path that should be explored for React Native as it vastly improves the developer experience. There are, however, some questions which would need to be addressed:
app/react-native/module and detect when we are displaying on an iPad to enable this behaviour, or do we create an entirely new module which developers opt-in to using?storybook-uimodule and a possible React Native implementation? Would we need to start again, or is it possible to reuse some of the existing state management? Do we even need to share any of this?