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

refactor: Refactor OndeviceUI components to function components #189

Conversation

lauriharpf
Copy link
Contributor

@lauriharpf lauriharpf commented Jun 27, 2021

Issue: #179

What I did

Refactored components under app/react-native/src to function components, with the exception of Addons.tsx (done in #187).

This also updates Podfile.lock in the examples/native as a result of running pod install for iOS.

How to test

Run the native examples and check that the Storybook UI works correctly.

  • Does this need a new example in examples/native? In my opinion, no
  • Does this need an update to the documentation? In my opinion, no

…UI to functional components, with the exception of absolute-positioned-keyboard-aware-view.tsx, OnDeviceUI.tsx (to be done later) and Addons.tsx (done in another PR). Updated Podfile.lock for the react-native example project
…move the React.memo() calls to the export to reduce nesting.
@dannyhw
Copy link
Member

dannyhw commented Jun 27, 2021

@lauriharpf sorry I haven't got around to reviewing your changes on #187 yet, not had the time . I'll be able to do it this coming week though for sure, including this one :).

Thanks again for your contribution.

@lauriharpf
Copy link
Contributor Author

@lauriharpf sorry I haven't got around to reviewing your changes on #187 yet, not had the time . I'll be able to do it this coming week though for sure, including this one :).

Thanks again for your contribution.

Thanks! No worries, there are many other things in life in addition to maintaining open source projects 🙂 . Your quick responses and welcoming attitude are much appreciated!

@lauriharpf
Copy link
Contributor Author

Noticed that out of the components under app/react-native there was only one class component (StoryListView) remaining. Wasn't a big job to convert, so did it in 7d9495f which is now included in this PR.

Not currently planning to touch the addon components as it might conflict quite a bit with #182 . Please ping if there's something that I can do to help out 🙂 .

@dannyhw
Copy link
Member

dannyhw commented Jun 29, 2021

@lauriharpf I'm actually thinking of pushing the current changes in #182 so that it will be easier to collaborate on the addon changes. It will probably be a little bit unfinished/broken temporarily however #182 is getting too big to deal with currently so I think it would make sense to split the controls feature into parts. Does that seem reasonable to you? Depending on feedback I might merge that later tonight after a few changes.

@lauriharpf
Copy link
Contributor Author

@lauriharpf I'm actually thinking of pushing the current changes in #182 so that it will be easier to collaborate on the addon changes. It will probably be a little bit unfinished/broken temporarily however #182 is getting too big to deal with currently so I think it would make sense to split the controls feature into parts. Does that seem reasonable to you? Depending on feedback I might merge that later tonight after a few changes.

@dannyhw , that sounds like a good idea. Not sure about the state of #182, but if merging it wouldn't break any functionality then it sounds wise to merge 👍. If merging would cause severe regressions or add a great deal of technical debt, then we could also try working on #182 together: If there are clearly separate tasks you'd like to get done, I could branch off #182 and then make a PR back to #182.

Would go with whatever helps you out the most & helps get the first 6.X-based release out the door fastest, even if it would be a pretty crude alpha release 🙂 .

Copy link
Member

@dannyhw dannyhw left a comment

Choose a reason for hiding this comment

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

Looks good to me!
I also ran it on my machine and it seemed fine.

Thanks for your contribution 🙇

@dannyhw dannyhw merged commit a4eb9f8 into storybookjs:next-6.0 Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants