-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD for payment 2024-01-18] [HOLD for payment 2023-10-23] Refactor Form to a custom hook #25397
Comments
Triggered auto assignment to @strepanier03 ( |
Triggered auto assignment to Design team member for new feature review - @shawnborton ( |
No design changes are necessary here. Removing @shawnborton as an assignee |
Hi it's Lucas from Callstack. Happy to have a go on that one :) I assume, given the app is currently being migrated to TS, this new custom form hook should be written in TS too? |
Thanks @lukemorawski! Assigned you to the issue. Writing it in TS sounds good. |
Cool! Thanks @luacmartins. Starting the research :) |
Hey @lukemorawski, it seems like another external contributor is already working on this issue. I'll unassign you from it, sorry about the confusion. |
Hello, I'm from Software Mansion, and I would like to work on this task. |
@luacmartins Cool, though it was a fun thing to work with :) Good luck! |
Hey! During solving another issue, we found a warning/error which has its root in the |
As BZ, I'll follow this is being done and handle any actions or payment as needed. Feel free to ping me if I missed taking a needed action. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Form component causes the app crashing if there is any child which uses hooks API. What is the root cause of that problem?React.cloneElement is a legacy API and it doesn't support hooks. What changes do you think we should make in order to solve the problem?We can introduce the What alternative solutions did you explore? (Optional)Instead of Second alternativeWe can introduce a third party library like formik, or react-hook-form - both have good support and big community (over 30k stars on GitHub and couple millions of weekly downloads). |
Curious for your thoughts on the proposal above @marcaaron @tgolen |
Looks kinda neat! Here are some of my initial thoughts:
|
Sorry just skimmed here, but the proposal is to switch to We considered this actually. There were two downsides:
1 is no longer a problem 🎉 Our existing design looks better to me. But that's just my personal preference. I am more passionate that we have consistent UX practices here. As I briefly mentioned someplace else (Slack maybe) I wouldn't stand in anyone's way if they really want to do this, but not passionate about changing this right now. |
@luacmartins Do we have the issue to migrate this one ?https://github.com/Expensify/App/blob/main/src/components/AddressForm.js |
It should be this issue - #30312 |
@luacmartins I just checked the PR that related to the issue you mentioned above but it does not contain the https://github.com/Expensify/App/blob/main/src/components/AddressForm.js |
Ah you're right, we refactored ReimbursementAccount/AddressForm in that one, not components/AddressForm. Created a new issue for it here. Thanks for catching that. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.24-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-01-18. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Problem
Although our Form component has been working great so far, the migration to functional components has surfaced some issues with our design. Coming from this thread, our existing Form component either causes child functional components with hooks to crash the App or functional components without hooks don't work. Given that we're migrating the whole codebase to use hooks, we need to redesign Form to play nicely with child components with/without hooks.
Why is this important
We migrated to functional components and we need Form to work with child functional components that have hooks.
Solution
Redesign Form into a custom hook. This will be worked on by an external agency.
The text was updated successfully, but these errors were encountered: