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

Storybook install, stories and guidelines #3374

Merged
merged 5 commits into from
Feb 1, 2022

Conversation

georgewrmarshall
Copy link
Contributor

@georgewrmarshall georgewrmarshall commented Nov 4, 2021

Description
Initial install of react native storybook as well as some initial stories and guidelines. MVP for react native storybook requires some manual steps(also suggested in the rn storybook readme) that could be improved in later versions.

To get storybook working in the current state of this PR you have to follow these steps. I have also outlined them in the documentation guidelines included in this PR

  1. In the root ./index.js file comment out AppRegistry.registerComponent(name, () => Root); and add:
export { default } from "./storybook";

this will replace the entry point of the app with storybook.

  1. Once you have replaced the entry point of the app with storybook in ./index.js run
yarn watch
  1. Open a new terminal window and run
yarn start:ios

You should see storybook navigator running in your web browser and the components rendering on the simulator

storybook.rn.720.mov

TO DO

  • Resolve postinstall issue with rn-nodeify
  • Allow storybook to be easily used in developer mode without replacing the content of any files. Other options This example has a great developer experience Have explored other DX improvements but because of the way our app works I ran into a few issues so starting with this implementation as MVP
  • Update DOCUMENTATION_GUIDELINES.md once the developer experience has been improved. Docs have been updated with how to get storybook running with current implementation

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable NA
  • Any added code is fully documented

Issue
Resolves #2230

@georgewrmarshall georgewrmarshall added the DO-NOT-MERGE Pull requests that should not be merged label Nov 4, 2021
@georgewrmarshall georgewrmarshall self-assigned this Nov 4, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2021

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@georgewrmarshall georgewrmarshall changed the title Storybook install, stories and guidelines Storybook install, stories and guidelines (WIP) Nov 4, 2021
ios/Podfile.lock Outdated Show resolved Hide resolved
@georgewrmarshall
Copy link
Contributor Author

Hey @dannyhw! Do you have any suggestions on how we could address the rn-nodeify and storybook issue we are having? Storybook works if we comment out this step in our post install process. Have you seen this before?

@dannyhw
Copy link

dannyhw commented Jan 12, 2022

@georgewrmarshall I'll take a look. Might need some help understanding the setup you have though.

@dannyhw
Copy link

dannyhw commented Jan 12, 2022

@georgewrmarshall why do you need the nodeify step?

@georgewrmarshall
Copy link
Contributor Author

georgewrmarshall commented Jan 12, 2022

@georgewrmarshall why do you need the nodeify step?

@Cal-L or @rickycodes could you elaborate on this? @dannyhw I think it's because rn-nodeify installs node packages that aren't available in react native and some of the dependencies require them?

@dannyhw
Copy link

dannyhw commented Jan 12, 2022

hmm yeah I see, well I don't think it should affect anything. Although i think crypto is used somewhere to generate ids or something in an upstream package in storybook. What specifically happens when it fails? whats the error?

@georgewrmarshall
Copy link
Contributor Author

georgewrmarshall commented Jan 12, 2022

I've outlined the error in this issue which you commented on previously. I also found this old issue which may give a clearer description on how to reproduce it.

@dannyhw
Copy link

dannyhw commented Jan 13, 2022

@georgewrmarshall I ran the project and the storybook and I didn't have any issue when running just the ondevice portion.

Everything seems fine until you run the react-native-server, at that point I could reproduce the issue on your project. I wonder if the server is a requirement for you? I ask that because the server is optional and not required to run storybook on the device. if its not part of your requirements to be able to control the device UI from the browser then you could just avoid the issue entirely by using storybook from the on device UI.

If I were to guess I suspect you are using some library that is clashing with the server in some way. Maybe using nodeify breaks something for webpack?

Unfortunately the log here doesn't really say much to me since this enchanced resolve package is used by webpack and not storybook directly as far as I can tell.

/Users/danny/workspace/metamask-mobile/node_modules/enhanced-resolve/lib/Resolver.js:301
                const idxQuery = identifier.indexOf("?");

Webpack is used to bundle the code for the server of course but if anything I guess the issue is with the setup of the manager which is part of the storybook app on the web which itself is an upstream dependency.

The server was built a really long time ago by someone else and it is a mystery to me currently. My focus has been more on the ondeviceUI since thats how I use it. Anyone who worked on the server is long gone from the project unfortunately. I have plans to attempt to replace the server with something compatible with newer versions of storybook but that won't be for some time and 6.0 will likely release without the server.

My suggestion would be to ignore the server unless you specifically want to use it for some purpose, that depends on your needs though.

@georgewrmarshall
Copy link
Contributor Author

Hey @dannyhw thank you so much for looking into this. The storybook server is a "nice to have" for navigating stories and interacting with knobs but by I can definitely live without it if it means that we can get storybook working on device. I'll continue with your suggestion and remove storybook server. Thanks again 🙏 💯

@dannyhw
Copy link

dannyhw commented Jan 13, 2022

No problem :). Let me know if theres anything else I can do to help. Also if you ever want to try out the 6.0 currently in alpha then let me know and I'd be happy to help with that.

@georgewrmarshall georgewrmarshall removed help wanted DO-NOT-MERGE Pull requests that should not be merged labels Jan 24, 2022
@georgewrmarshall georgewrmarshall added the DO-NOT-MERGE Pull requests that should not be merged label Jan 24, 2022
@georgewrmarshall georgewrmarshall added area-documentation and removed DO-NOT-MERGE Pull requests that should not be merged labels Jan 27, 2022
@georgewrmarshall georgewrmarshall changed the base branch from develop to main January 27, 2022 00:13
@georgewrmarshall georgewrmarshall marked this pull request as ready for review January 27, 2022 00:14
@georgewrmarshall georgewrmarshall requested a review from a team as a code owner January 27, 2022 00:14
@georgewrmarshall georgewrmarshall changed the title Storybook install, stories and guidelines (WIP) Storybook install, stories and guidelines Jan 27, 2022
@georgewrmarshall georgewrmarshall added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 27, 2022
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

storybook/index.js Outdated Show resolved Hide resolved
@georgewrmarshall georgewrmarshall force-pushed the docs/2230-implement-storybook branch 2 times, most recently from 49218cb to d78935e Compare January 31, 2022 23:11
@georgewrmarshall georgewrmarshall removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Feb 1, 2022
@georgewrmarshall georgewrmarshall merged commit e14bd71 into main Feb 1, 2022
@georgewrmarshall georgewrmarshall deleted the docs/2230-implement-storybook branch February 1, 2022 18:08
@github-actions github-actions bot locked and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement storybook for a better view/usage of our shared components
4 participants