search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.58k stars 2.92k forks source link

[HOLD for payment 2022-12-08] [$250][Form Refactor] CompanyStep #9580

Closed luacmartins closed 1 year ago

luacmartins commented 2 years ago

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

Here's an example of a Form refactor: https://github.com/Expensify/App/pull/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:

melvin-bot[bot] commented 2 years ago

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

adelekennedy commented 2 years ago

@luacmartins is this something contributors can work on? Double checking before I open an Upwork job

luacmartins commented 2 years ago

Yes! The Form refactor project is all open to contributors :)

luacmartins commented 2 years ago

This issue is being put on hold due to push to page discussions, as per this comment.

adelekennedy commented 2 years ago

Ty ty!

adelekennedy commented 2 years ago

on hold

adelekennedy commented 2 years ago

on hold

adelekennedy commented 2 years ago

on hold

luacmartins commented 2 years ago

@adelekennedy removing hold! We can post this job on upwork and get contributors to start working on it!

adelekennedy commented 2 years ago

let's gooooo

adelekennedy commented 2 years ago

internal external

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

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

ravindra-encoresky commented 2 years ago

Approach Proposal

Here are my finding to fix this -

remove props.png

add inputID prop.png

mananjadhav commented 2 years ago

@ravindra-encoresky Thanks for the proposal but it is incomplete. The CompanyStep is a part of the step navigation, are you sure it is working fine with your changes? Also can you check the guidelines mentioned in the GH body.

Still open for better proposals.

melvin-bot[bot] commented 2 years ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

ravindra-encoresky commented 2 years ago

@ravindra-encoresky Thanks for the proposal but it is incomplete. The CompanyStep is a part of the step navigation, are you sure it is working fine with your changes? Also can you check the guidelines mentioned in the GH body.

Still open for better proposals.

Okay, thanks.

Will submit another soon.

ravindra-encoresky commented 2 years ago

Updated proposed solution :

  1. CompanyStep.js is using ReimbursementAccountForm.js as the wrapper for form components. and ReimbursementAccountForm is used by many other files. So first step is to remove the dependency on ReimbursementAccountForm. Because if we refactor ReimbursementAccountForm then we need to refactor other files also. https://github.com/Expensify/App/blob/85a313891bcc4e247e947026febcd9b525d3406a/src/pages/ReimbursementAccount/CompanyStep.js#L203-L211

  2. Next, we use Form.js in place of FormScrollView.js

  3. Create the Company step form id in ONYXKEYS.js and pass it to the form component in CompanyStep.js https://github.com/Expensify/App/blob/85a313891bcc4e247e947026febcd9b525d3406a/src/ONYXKEYS.js#L183-L185

  4. Remove FormAlertWithSubmitButton and pass submit function to Form. https://github.com/Expensify/App/blob/85a313891bcc4e247e947026febcd9b525d3406a/src/pages/ReimbursementAccount/ReimbursementAccountForm.js#L54-L64

  5. Pass inputID to each form component and remove onChangeText, value, errorText props. Form.js will manage them

  6. Update validate function to use values from the Form component and return errors

  7. Remove state variables for inputs, They are not required now.

  8. Cleanup code

sketchydroide commented 2 years ago

@mananjadhav friendly bump (also removing Orvedue :) )

mananjadhav commented 2 years ago

@ravindra-encoresky I see that you've followed guidelines in the updated proposal. But it still has details missing on:

Thanks @sketchydroide for the bump. Sorry I missed this I thought it was the same proposal. Do you think it makes sense to double this one? Been open for 14 days now.

ravindra-encoresky commented 2 years ago

@ravindra-encoresky I see that you've followed guidelines in the updated proposal. But it still has details missing on:

  • You've mentioned remove the dependency of ReimbursementAccountForm Can you describe how are you planning to remove it?
  • As I've mentioned earlier this step is a part of wizard steps movement (where we show steps 1/5 and so on). I don't see that happening here. Please look at the Testing guidelines in the GH body as well.

Thanks @sketchydroide for the bump. Sorry I missed this I thought it was the same proposal. Do you think it makes sense to double this one? Been open for 14 days now.

@mananjadhav thanks for the replies, I conclude my proposal here -

Since ReimbursementAccountForm is a wrapper around form components in the companyStep and other wizard steps, it renders form components as children and a FormAlertWithSubmitButton inside FormScrollView.

Our idea is to replace ReimbursementAccountForm component with FormScrollView inside CompanyStep.js.

Then we will add FormAlertWithSubmitButton from ReimbursementAccountForm to CompanyStep.js and copy other error logic from ReimbursementAccountForm to CompanyStep as well.

mananjadhav commented 2 years ago

Thanks for the update @ravindra-encoresky.

@sketchydroide @ravindra-encoresky's Proposal looks good to me. We'll have to ensure that we follow the Testing and Implementation Guidelines.

🎀 👀 🎀  C+ reviewed

ravindra-encoresky commented 2 years ago

Thanks for the update @ravindra-encoresky.

@sketchydroide @ravindra-encoresky's Proposal looks good to me. We'll have to ensure that we follow the Testing and Implementation Guidelines.

🎀 👀 🎀  C+ reviewed

Thanks, I'll follow the guidelines.

How shall I proceed, should I submit a proposal on upwork ? please guide me through.

thanks.

mananjadhav commented 2 years ago

We wait for @sketchydroide to review it and give a 🟢 to your proposal. After that you can start with your PR and yeah meanwhile you can apply to Upwork. Please refer to these contributing guidelines as well before you start writing any code.

Hope this helps.

ravindra-encoresky commented 2 years ago

We wait for @sketchydroide to review it and give a 🟢 to your proposal. After that you can start with your PR and yeah meanwhile you can apply to Upwork. Please refer to these contributing guidelines as well before you start writing any code.

Hope this helps.

Hi guys, waiting for the review.

sketchydroide commented 2 years ago

sorry, been really busy with the releases, reviewing now

sketchydroide commented 2 years ago

looks good

melvin-bot[bot] commented 2 years ago

📣 @ravindra-encoresky You have been assigned to this job by @sketchydroide! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

ravindra-encoresky commented 2 years ago

📣 @ravindra-encoresky You have been assigned to this job by @sketchydroide! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

Great, yes applied on upwork already, can expect PR by Wednesday 31 Aug 2022.

ravindra-encoresky commented 2 years ago

hi @mananjadhav @sketchydroide @luacmartins @adelekennedy I've shared the PR but the job hasn't awarded on Upwork yet.

adelekennedy commented 2 years ago

@ravindra-encoresky hired you on Upwork!

mananjadhav commented 2 years ago

Coming from this comment, it seems AddressForm, doesn't work well with Form.js without modifying it.

We are tracking the AddressForm refactor, here https://github.com/Expensify/App/issues/10738. We've almost through one proposal, and hence I feel we should put this on hold until we merge #10738.

@sketchydroide What do you think?

sketchydroide commented 2 years ago

ahh sorry missed this. yeah lets put this on hold

sketchydroide commented 2 years ago

not overdue

mananjadhav commented 2 years ago

Proposal for the issue on hold was approved, so once that is worked upon and merged we can pick this.

sketchydroide commented 2 years ago

no overdue still waiting on the other issue

sketchydroide commented 2 years ago

I think we are still waiting?

mananjadhav commented 2 years ago

Yes we're still waiting.

mananjadhav commented 2 years ago

The PR for the issue holding this one is complete and almost ready to merge. This should be off hold in 1-2 days.

mananjadhav commented 2 years ago

@ravindra-encoresky @sketchydroide The linked issue PR is merged. @ravindra-encoresky You can take the latest pull from main branch and continue with the PR.

mananjadhav commented 2 years ago

@ravindra-encoresky Are you still available to pick this up?

@sketchydroide Can we remove the hold label?

mananjadhav commented 2 years ago

@ravindra-encoresky quick bump on this one.

ravindra-encoresky commented 2 years ago

@ravindra-encoresky Are you still available to pick this up?

@sketchydroide Can we remove the hold label?

This has already been done a month ago. On hold from very long from you. Whats next to pick ?

mananjadhav commented 2 years ago

@ravindra-encoresky The issue was on hold because AddressForm wasn't working with your PR. PR for the same is merged and now we can continue with your PR by picking the latest main changes.

ravindra-encoresky commented 2 years ago

@ravindra-encoresky The issue was on hold because AddressForm wasn't working with your PR. PR for the same is merged and now we can continue with your PR by picking the latest main changes.

okay thanks, eagerly waiting for this to finish.

mananjadhav commented 2 years ago

Thanks you can get started on this. Appreciate your patience on this one @ravindra-encoresky

luacmartins commented 2 years ago

This is off-hold!

melvin-bot[bot] commented 2 years ago

Current assignee @adelekennedy is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 2 years ago

Current assignee @mananjadhav is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 2 years ago

Current assignee @sketchydroide is eligible for the External assigner, not assigning anyone new.