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-05-20] [$500] Refactor StatePicker to be compatible with Form #7537

Closed
luacmartins opened this issue Feb 2, 2022 · 48 comments
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 Reviewing Has a PR in review Task

Comments

@luacmartins
Copy link
Contributor

luacmartins commented Feb 2, 2022

With the implementation of our new Form component we need to refactor StatePicker to be Form compatible. Here are the changes that need to be made:

  1. An optional isFormInput prop.
  2. If isFormInput is true, require a inputID prop. Use getInputIDPropTypes to enforce this propType rule.
  3. Add an optional shouldSaveDraft prop. Defaults to false.
  4. Make the value prop optional.
  5. In the input’s onBlur method, call props.onBlur().
  6. Rename the input's onChange method to onInputChange.
  7. In the input’s onInputChange method (or equivalent e.g. onTextChange, etc), call props.onInputChange().
  8. Remove the hasError prop.
  9. Add an optional errorText prop. Defaults to an empty string.
  10. Update the error message to display if errorText is truthy.
  11. Update the inline error display style so it is left aligned with the label and input value. See example image for TextInput:
    Screen Shot 2022-02-02 at 10 37 38 AM
  12. Make sure that props.ref is attached to the appropriate DOM node. This could involve forwarding the ref to a child component. This is important so we can call ref.focus().
  13. Add the input to the test Form stories and make sure that it works. You can check this by running npm run storybook and testing the component in the example form.
  14. Remove any unused code.

There's an example of a refactor to TextInput in this PR.

@luacmartins luacmartins added External Added to denote the issue can be worked on by a contributor Engineering Daily KSv2 Task labels Feb 2, 2022
@MelvinBot
Copy link

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

@MitchExpensify

This comment was marked as outdated.

@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 2, 2022
@MelvinBot
Copy link

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

@sig5
Copy link
Contributor

sig5 commented Feb 3, 2022

Proposal
The problem statement seems quite self-explanatory in regards of the changes needed. Can I possibly work on it? Thanks.

@parasharrajat
Copy link
Member

@sig5 Do you have questions before we move forward? I would suggest to give it another look and ask questions if any.

Though, I have one 😄

Add an optional shouldSaveDraft prop. Defaults to false.

@luacmartins Does it need to be implemented as well or just define a prop for now?

@sig5
Copy link
Contributor

sig5 commented Feb 3, 2022

Hi @parasharrajat , Please correct me if I am wrong or misunderstanding the problem, But I can observe, shouldsaveDraft is used to see if the form element needs to save value as the draft.
It is handled here.

App/src/components/Form.js

Lines 144 to 167 in 4c48227

// We clone the child passing down all form props
const inputID = child.props.inputID;
const defaultValue = this.props.draftValues[inputID] || child.props.defaultValue;
this.inputValues[inputID] = defaultValue;
return React.cloneElement(child, {
ref: node => this.inputRefs[inputID] = node,
defaultValue,
errorText: this.state.errors[inputID] || '',
onBlur: () => {
this.setTouchedInput(inputID);
this.validate(this.inputValues);
},
onChange: (value) => {
this.inputValues[inputID] = value;
if (child.props.shouldSaveDraft) {
FormActions.setDraftValues(this.props.formID, {[inputID]: value});
}
this.validate(this.inputValues);
},
});
});
}

So adding a propType in the component along with a defaultProp should suffice to trigger it save in Onyx, which can be fetched next time it is mounted.

@parasharrajat
Copy link
Member

parasharrajat commented Feb 3, 2022

Yeah, that's correct. Thanks for pointing that.

I think we can proceed with your proposal @sig5 if you don't have any questions.

cc: @thienlnam

🎀 👀 🎀 C+ reviewed

@sig5
Copy link
Contributor

sig5 commented Feb 3, 2022

I think I do not have any questions atm :D

@luacmartins
Copy link
Contributor Author

luacmartins commented Feb 3, 2022

Does it need to be implemented as well or just define a prop for now?

@sig5 comment is correct. We have already implemented that and should only need to pass shouldSaveDraft + call onChange in the input as described in the OP.

@thienlnam
Copy link
Contributor

Sending it! @MitchExpensify could you please hire @sig5 ?

@MitchExpensify
Copy link
Contributor

For sure! Can you please make a proposal on the Upwork job @sig5 ?

@sig5
Copy link
Contributor

sig5 commented Feb 4, 2022

Done!

@MitchExpensify
Copy link
Contributor

Thanks, hired @sig5 !

@sig5
Copy link
Contributor

sig5 commented Feb 5, 2022

Hey guys, I am coming across this error whilst adding the StatePicker to the storybook. Seems like a Transpilation error related to Flow. I am not sure how to tackle it.

RR! ./node_modules/react-native/Libraries/Components/UnimplementedViews/UnimplementedView.js 19:47
ERR! Module parse failed: Unexpected token (19:47)
ERR! You may need an appropriate loader to handle this file type, currently, no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
ERR! |  * View component and renders its children.
ERR! |  */
ERR! > class UnimplementedView extends React.Component<$FlowFixMeProps> {
ERR! |   render(): React.Node {
ERR! |     // Workaround require cycle from requireNativeComponent
ERR!  @ ./node_modules/@react-native-picker/picker/js/PickerIOS.js 2:0-103 5:42-59 12:17-34
ERR!  @ ./node_modules/@react-native-picker/picker/js/index.js
ERR!  @ ./node_modules/react-native-picker-select/src/index.js
ERR!  @ ./src/components/Picker/BasePicker/index.js
ERR!  @ ./src/components/Picker/index.js
ERR!  @ ./src/components/StatePicker.js
ERR!  @ ./src/stories/Form.stories.js
ERR!  @ ./src sync ^\.(?:(?:^|\/|(?:(?:(?!(?:^|\/)\.).)*?)\/)(?!\.)(?=.)[^/]*?\.stories\.(js|jsx|ts|tsx))$

This seems like a relevant issue: storybookjs/storybook#10131
Can someone suggest changes for how to integrate Flow support for the storybook module? ( Or tag someone? )

@luacmartins
Copy link
Contributor Author

Asked in Slack.

@rushatgabhane
Copy link
Member

@sig5 @parasharrajat Before starting on this one, we need to first merge a PR for Refactor Picker right?

@sig5
Copy link
Contributor

sig5 commented Feb 9, 2022

Yes Statepicker uses Picker Component under the hood, so having those changes done first would be ideal.

@parasharrajat
Copy link
Member

Good eye @rushatgabhane. We are already waiting for some issue @sig5 is having.

@luacmartins luacmartins removed Daily KSv2 Internal Requires API changes or must be handled by Expensify staff labels Apr 22, 2022
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Apr 22, 2022
@melvin-bot
Copy link

melvin-bot bot commented Apr 22, 2022

📣 @parasharrajat You have been assigned to this job by @luacmartins!
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 📖

@melvin-bot
Copy link

melvin-bot bot commented Apr 22, 2022

📣 @jayeshmangwani You have been assigned to this job by @luacmartins!
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 📖

@luacmartins luacmartins added the Weekly KSv2 label Apr 22, 2022
@parasharrajat
Copy link
Member

parasharrajat commented Apr 22, 2022

Ok Great. @jayeshmangwani this is urgent so I am hoping that PR will be ready by Monday. Thanks for taking it up.

🎀 👀 🎀 C+ reviewed

@MitchExpensify
Copy link
Contributor

@jayeshmangwani Can you apply to this Upwork Job please? I had to abandon the original job so we'll use this new posting to issue payment (invited you to the new job @parasharrajat)

@jayeshmangwani
Copy link
Contributor

@jayeshmangwani Can you apply to this Upwork Job please? I had to abandon the original job so we'll use this new posting to issue payment (invited you to the new job @parasharrajat)

Sure I am applying

@jayeshmangwani
Copy link
Contributor

Ok Great. @jayeshmangwani this is urgent so I am hoping that PR will be ready by Monday. Thanks for taking it up.

🎀 👀 🎀 C+ reviewed

Sure 👍

@jayeshmangwani
Copy link
Contributor

PR raised
@parasharrajat

@luacmartins luacmartins added the Reviewing Has a PR in review label Apr 27, 2022
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 13, 2022
@melvin-bot melvin-bot bot changed the title [$500] Refactor StatePicker to be compatible with Form [HOLD for payment 2022-05-20] [$500] Refactor StatePicker to be compatible with Form May 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 13, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.57-17 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-05-20. 🎊

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 19, 2022
@MitchExpensify
Copy link
Contributor

https://www.upwork.com/jobs/~0116994c22a1b60fb6 mind applying here @parasharrajat so I can issue C+ payment?

@MitchExpensify
Copy link
Contributor

Paid @jayeshmangwani , thank you very much!

@jayeshmangwani
Copy link
Contributor

@MitchExpensify Thankyou

@parasharrajat
Copy link
Member

@MitchExpensify Done.

@MitchExpensify
Copy link
Contributor

Done! Thanks all

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 Reviewing Has a PR in review Task
Projects
None yet
Development

No branches or pull requests

10 participants