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

[HOLD for payment 2022-10-12] [$1000][Form Refactor] BankAccountStep #9579

Closed
8 tasks
luacmartins opened this issue Jun 27, 2022 · 60 comments
Closed
8 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review

Comments

@luacmartins
Copy link
Contributor

luacmartins commented Jun 27, 2022

Coming from the New Expensify Forms design doc, we should refactor BankAccountStep to use the new form component, follow the guidelines below:

Here's an example of a Form refactor: #9056

Guidelines

  1. Replace the form component with Form.js.
  2. Create a unique Onyx key in ONYXKEYS.FORM and pass it as the formID prop to Form.
  3. Pass a validate callback prop.
  4. Pass an onSubmit callback prop that calls the API via an action.
  5. Update all inputs wrapped by Form, following the guidelines in Refactor inputs.
  6. Remove any unused code.

Testing

Verify that:

  • UI looks as it did before the refactor
  • Values can be added and edited
  • Errors are highlighted correctly (input border)
  • Error messages show up correctly
  • Draft values are saved properly
  • Form alerts are displayed correctly
  • Clicking the fix the errors link focuses the first input with error
  • No duplicate submission of the form occurs (when it's already submitting)

Job Post: https://www.upwork.com/jobs/~015df14d7f44106b74

@luacmartins luacmartins added External Added to denote the issue can be worked on by a contributor Engineering Daily KSv2 labels Jun 27, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2022

Triggered auto assignment to @kevinksullivan (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@liyamahendra
Copy link
Contributor

@luacmartins I did a similar work on #9234 and would be happy to take this up.

Since this isn't an usual bug, should I propose the changes required for making the BankAccountStep to use Form or you expect something else as a part of the proposal?

@luacmartins
Copy link
Contributor Author

@liyamahendra the issue is self-explanatory so there's no proposal needed, just your interest in working on this!

Let's wait for the job to be exported to that we can follow the process though 😉

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jun 28, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 28, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (Exported)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 28, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 28, 2022

Triggered auto assignment to @iwiznia (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title [Form Refactor] BankAccountStep [$250] [Form Refactor] BankAccountStep Jun 28, 2022
@iwiznia
Copy link
Contributor

iwiznia commented Jun 28, 2022

hmmm maybe we should pause this till we discuss push to page?

@trjExpensify
Copy link
Contributor

^^ Agreed. Seems like we don't need to double up on refactoring and then potentially refactoring it all over again with push to page, unless I'm missing something?

@kevinksullivan
Copy link
Contributor

Oh... I agree. This will change as a result of the explorations we're working on. :nobodyreads: sorry @luacmartins , I agree we should hold on this.

@kevinksullivan kevinksullivan removed Exported External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Jun 28, 2022
@luacmartins
Copy link
Contributor Author

Fair enough! When do we plan to discuss push to page?

@luacmartins luacmartins changed the title [$250] [Form Refactor] BankAccountStep [$250] [HOLD] [Form Refactor] BankAccountStep Jun 29, 2022
@luacmartins luacmartins changed the title [$250] [HOLD] [Form Refactor] BankAccountStep [HOLD] [$250][Form Refactor] BankAccountStep Jun 29, 2022
@jayeshmangwani
Copy link
Contributor

@dylanexpensify Applied 🚀

@jayeshmangwani
Copy link
Contributor

PR created

cc: @mananjadhav @iwiznia

@melvin-bot melvin-bot bot added the Overdue label Sep 8, 2022
@dylanexpensify
Copy link
Contributor

Not overdue Mel

@melvin-bot melvin-bot bot removed the Overdue label Sep 8, 2022
@mananjadhav
Copy link
Collaborator

Thanks for the PR @jayeshmangwani I'll get it to tomorrow.

@melvin-bot
Copy link

melvin-bot bot commented Sep 12, 2022

@iwiznia, @mananjadhav, @dylanexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Sep 12, 2022
@dylanexpensify
Copy link
Contributor

Any updates @mananjadhav ?

@melvin-bot melvin-bot bot removed the Overdue label Sep 12, 2022
@mananjadhav
Copy link
Collaborator

PR is under review, and I've raised 1-2 comments on the PR here

@melvin-bot melvin-bot bot added the Overdue label Sep 15, 2022
@dylanexpensify
Copy link
Contributor

Should we move to weekly?

@melvin-bot melvin-bot bot removed the Overdue label Sep 15, 2022
@luacmartins luacmartins added Weekly KSv2 Reviewing Has a PR in review and removed Daily KSv2 labels Sep 15, 2022
@luacmartins
Copy link
Contributor Author

Moved to weekly and added the reviewing label

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 5, 2022
@melvin-bot melvin-bot bot changed the title [$1000][Form Refactor] BankAccountStep [HOLD for payment 2022-10-12] [$1000][Form Refactor] BankAccountStep Oct 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.11-4 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 2022-10-12. 🎊

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 12, 2022
@dylanexpensify
Copy link
Contributor

payments sent, contracts ended, job closed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests