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

Framework: Add native storybook for components. #17829

Closed
wants to merge 1 commit into from

Conversation

epiqueras
Copy link
Contributor

Description

This PR adds Storybook for React Native and uses it to create a native version of the @wordpress/components storybook.

This will be super useful for testing cross platform compatibility of components.

The new storybook can be started with npm run design-system:dev:native.

How has this been tested?

It was verified that the web storybook still works as expected and that so does npm run design-system:dev:native.

Screenshots

Screen Shot 2019-10-07 at 2 12 34 PM

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@epiqueras epiqueras added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Build Tooling Issues or PRs related to build tooling [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Oct 7, 2019
@epiqueras epiqueras added this to the Future milestone Oct 7, 2019
@epiqueras epiqueras self-assigned this Oct 7, 2019
@gziolo gziolo requested review from pinarol, hypest and koke October 8, 2019 06:58
@gziolo gziolo added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Oct 8, 2019
@gziolo
Copy link
Member

gziolo commented Oct 8, 2019

Nice one 😍

Based on my previous research and the screenshot included, this still requires a React Native project to be included in the repository so we could run it and embed the Storybook there. As far as I learned, there are 2 strategies:

  • the one explained in the screenshot where you bootstrap an example app
  • you enabled it as one of the screens in the existing app behind the development flag

@hypest @koke @pinarol - what would be the preferred approach?

@koke
Copy link
Contributor

koke commented Oct 8, 2019

If I understand it correctly, both approaches could work, but I think having an example app might be less problematic for everyone, at least until we can make the developer onboarding smoother in the current apps

@hypest
Copy link
Contributor

hypest commented Oct 8, 2019

what would be the preferred approach?

I'm assuming that we want to have the native variants of the components running on the device so, we will have to have the full native dev pipeline in place, including the Aztec editor that powers the RichText component. I think an example app will end up replicating much if not all of the https://github.com/wordpress-mobile/gutenberg-mobile setup so, I'd probably wait until we merge that demo app in Gutenberg's repo first. Thoughts?

@gziolo
Copy link
Member

gziolo commented Oct 8, 2019

I'm assuming that we want to have the native variants of the components running on the device so, we will have to have the full native dev pipeline in place, including the Aztec editor that powers the RichText component. I think an example app will end up replicating much if not all of the https://github.com/wordpress-mobile/gutenberg-mobile setup so, I'd probably wait until we merge that demo app in Gutenberg's repo first. Thoughts?

We can start small with covering only UI components which don't have deep native dependencies like Button component :)

@etoledom
Copy link
Contributor

etoledom commented Oct 8, 2019

We can start small with covering only UI components which don't have deep native dependencies like Button component :)

A simple react-native example app/project on gutenberg repo might be a good first step to implementing the mono-repo with gutenberg-mobile. Specially if it works good enough for testing simpler component that doesn't use Aztec or the native Bridge.

@epiqueras
Copy link
Contributor Author

Based on my previous research and the screenshot included, this still requires a React Native project to be included in the repository

This just runs the server for the mobile client to connect to. The code for the client can be anywhere, it just needs to connect to the machine running this server.

A simple react-native example app/project on gutenberg repo

Yes, we should start with a simple build process that can start a React Native development client to run the native storybook for @wordpress/components.

@hypest
Copy link
Contributor

hypest commented Oct 9, 2019

Yes, we should start with a simple build process that can start a React Native development client to run the native storybook for @wordpress/components

Is anyone already on that task? If yes, it would be ideal to collaborate with @dratwas that will be working these days on the "monorepo" plan (to merge the gutenberg-mobile repo to Gutenberg's) so the efforts are not replicated or diverging too much.

@gziolo
Copy link
Member

gziolo commented Oct 9, 2019

Let's review it as is then and add native apps integration as soon as we can celebrate all native bits moved to this repository from https://github.com/wordpress-mobile/gutenberg-mobile.

package.json Show resolved Hide resolved
@gziolo gziolo added the Storybook Storybook and its stories for components label Oct 10, 2019
@epiqueras
Copy link
Contributor Author

@gziolo This is good to go right?

@gziolo
Copy link
Member

gziolo commented Nov 21, 2019

@gziolo This is good to go right?

I didn't have a chance to play with it. It looks like it's outdated now. I see conflicting files listed.

I guess @hypest @pinarol or @koke should have better idea if this is useful in the current shape.

@koke
Copy link
Contributor

koke commented Nov 21, 2019

I've been trying to test this one, but I don't know how to make it work. Storybook seems to listen on port 7007, and react-native would try to connect to 8081. As far as I can see, changing the port that react-native tries to connect to can only be done by modifying the react-native sources.

Considering that 7007 is the default for storybook-native, I feel like I'm missing something really obvious here 🤔

@epiqueras
Copy link
Contributor Author

@koke 7007 is the port for the web interface that accompanies the app. The bundler is still on 8081.

Here is a guide that might be of help: https://pusher.com/tutorials/storybook-react-native.

@koke
Copy link
Contributor

koke commented Nov 21, 2019

Oh I see, that article was helpful. What's confusing is that npm run design-system:dev:native is not starting the bundler:

koke:~/ $ lsof -nP -i4TCP | grep 'node.*LISTEN'
node      58927 koke   24u  IPv4 0x61a4feb05fbf436b      0t0  TCP 127.0.0.1:7007 (LISTEN)

That article seems to assume you still run the bundler separately (react native run-platform will starting if not running already) and that the app will somehow load the storybook instead of the regular app.

@epiqueras
Copy link
Contributor Author

Yeah, we would need to have the mobile build process in here to integrate it all into a single command.

Or, I could add a mini standalone RN app, but I feel like that could make it harder to eventually bring in the mobile repo.

@koke
Copy link
Contributor

koke commented Nov 21, 2019

I've been trying to add a sample app just to test it, and the metro bundler doesn't like running any code outside it's root directory. I think the code in the PR would be OK to merge, but won't really be useful until we can figure out the right layout to launch.

I did one last attempt by running it from within gutenberg-mobile and trying to just import '@wordpress/components/storybook';, but I'm getting an error:

_$$_REQUIRE.context is not a function. (In '_$$_REQUIRE.context('../docs', true, /\/.+\.mdx$/)', '_$$_REQUIRE.context' is undefined)

I think require.context is specific to webpack, and metro's flavor of require doesn't know how to do that. I guess that's why that article is using react-native-storybook-loader.

@epiqueras
Copy link
Contributor Author

I think require.context is specific to webpack, and metro's flavor of require doesn't know how to do that.

Yeah, we'll need to polyfill it.

I think the code in the PR would be OK to merge, but won't really be useful until we can figure out the right layout to launch.

We might as well just wait until the mobile code is merged in then. I'll close this for now.

@epiqueras epiqueras closed this Nov 21, 2019
@aristath aristath deleted the try/native-storybooking branch November 10, 2020 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system Framework Issues related to broader framework topics, especially as it relates to javascript Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Package] Components /packages/components Storybook Storybook and its stories for components [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants