-
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
Enable again all address fields and allow the user to manually fill them / Move form errors to component state #6200
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some early feedback and questions, also going to request reviews from @marcaaron @luacmartins and @sketchydroide to get their thoughts on this approach since they are also working on the new forms project and this will be a component used in multiple forms.
src/components/AddressSearch.js
Outdated
|
||
googlePlacesRef.current.setAddressText(props.value); | ||
}, []); | ||
const [skippedFirstOnChangeText, setSkippedFirstOnChangeText] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment to explain what this is doing? I haven't seen this before and it isn't used anywhere else in the codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that is how you keep states in functional components. I think it isn't anywhere in the code because when there is a state, we use class components. In this case, there was some discussion about difficulties in converting this component into a class component.. will look for that.
If this stays like this, I'll add the comment you are requesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is the context cc @Luke9389 https://expensify.slack.com/archives/C03TQ48KC/p1634088400387400
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if we're removing the ref and useEffect()
then this should be converted back into a class component maybe 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can give it a try to converting it to class component now then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried converting it to a class component and had issues using local state with the library. We could definitely give it a try, but watch out for that.
src/components/AddressSearch.js
Outdated
if (!_.isEmpty(googlePlacesRef.current.getAddressText()) && !isTextValid) { | ||
saveLocationDetails({}); | ||
// This line of code https://github.com/FaridSafi/react-native-google-places-autocomplete/blob/47d7223dd48f85da97e80a0729a985bbbcee353f/GooglePlacesAutocomplete.js#L148 | ||
// will call onChangeText passing '' after the component renders the first time, clearing its value. Why? who knows, but we have to skip it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wasn't this an issue before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing that the removed code:
const googlePlacesRef = useRef();
useEffect(() => {
if (!googlePlacesRef.current) {
return;
}
googlePlacesRef.current.setAddressText(props.value);
}, []);
may have been fixing it. I would guess that this useEffect
set the value after it was cleared by the library. I will confirm that.
onChangeText={value => this.clearErrorAndSetValue('addressCity', value)} | ||
value={this.state.addressCity} | ||
errorText={this.getErrorText('addressCity')} | ||
translateX={-124} // TODO: Is this necessary? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prop is usually used for native platforms so the label value animates to the correct place when focusing and unfocusing the input. You can compare how it behaves with other inputs to see the difference when it isn't set, but 124 seems like a super high value here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, it used to be 14 I think, I messed up the value because I was playing with it to see if there was any difference.
Thanks for the explanation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a few more comments. As far as the forms convo goes...
This field is really interesting because it returns values for multiple fields. By pre-filling it with the addressStreet
now I'd expect the onChangeText
will just return the updated addressStreet
value. But it actually can return any number of address parts. That feels like a tough design to work into standard "form field" interface if we are hoping to cleanly plug this into a "big form design".
src/components/AddressSearch.js
Outdated
props.onChangeText('addressState', state); | ||
} | ||
if (zipCode) { | ||
props.onChangeText('addressZipCode', zipCode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this wasn't added in this PR, but dislike this design. The onChangeText
can get called multiple times with different key/values and it's really unusual. More typical would be a single text input that returns a text value for one field only.
Does it feel like we should be passing back an object or something here to just one callback and then letting the parent component determine what "fields" should be using the data?
I think then we would not need to worry about if these values exist just do
props.onLocationSelected({city, state, zipCode, streetNumberAndName});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree with this
src/components/AddressSearch.js
Outdated
props.onChangeText('addressState', null); | ||
props.onChangeText('addressZipCode', null); | ||
if (streetNumberAndName.length > props.value.length) { | ||
// We only replace what the user typed in the address input if we think ours is more complete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "length" the marker of "completeness"? I'm having trouble following why we're looking at the lengths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is because of this: https://expensify.slack.com/archives/C03TQ48KC/p1635967043023300?thread_ts=1635812089.454700&cid=C03TQ48KC
If the user types "123 Some Street #3232" (including the apt number), he will probably get a result like "123 Some Street". If the user clicks the result, we want try to avoid overwriting something that was more complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok got it. I think the "length" was throwing me a bit since you could have a bunch of whitespace in this field making it look "more detailed" but that's probably uncommon and your explanation makes sense.
src/components/AddressSearch.js
Outdated
|
||
googlePlacesRef.current.setAddressText(props.value); | ||
}, []); | ||
const [skippedFirstOnChangeText, setSkippedFirstOnChangeText] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried converting it to a class component and had issues using local state with the library. We could definitely give it a try, but watch out for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General logic looks good. I think we should definitely address the callback comment Marc left.
Update:
New problem Because we are using separate validation again for the fields Screen.Recording.2021-11-08.at.5.27.25.PM.movThe current implementation fails because to clear an error we copy the errors object and clear the corresponding key, but by the second and third, etc, we start copying and outdated copy of the errors object. This is solved in react states by setting a states using a callback. The callback gets the most up to date state and you can modify it. Onyx doesn't seem to have such mechanism. Proposal Stop storing the form error in |
I just noticed that we got some duplicated work going for different solutions for the same problem: PR: #6210 GH issues: Expensify/Expensify#183067 This solution is implementing a switch to choose between inputing all the address manually using separate fields or using the autocomplete. What should we do at this point? |
Hmm I really think this solution is superior to the others, since it allows for clearer flexibility with multiple fields , without some weird in-line edit workaround for the entire address. I think wee should go forward with this and revert the others, if necessary. |
Thread here for more context: https://expensify.slack.com/archives/C03TQ48KC/p1636488525194600?thread_ts=1635812089.454700&cid=C03TQ48KC (internal). I agree that opening a new issue would be good so we have something to reference. |
New Issue: #6309 |
I don't see a reason to keep these form errors in Onyx: there is no use in persisting them outside of the life of the form. Onyx doesn't support a way of merging a value using a callback in the same way as setState. The AddressSearch component can make us clear an error of several fields at the same time. Without the callback mode to set a state, clearing the error of multiple fields individually one after the other causes the errors not to be properly cleared (subsequent calls keep using the old state).
Update: Had a discussion with @luacmartins about this problem: #6200 (comment) We concluded that there wasn't a good reason to keep the form validation errors in Onyx rather than in the form's state. After all, there is no use for those errors when the component is not there. Putting the errors in the components state allows us to use setState which can be safely call multiple times to clear errors one by one if needed. Code changes: I moved the form validation errors from Onyx (ONYXKEYS.REIMBURSEMENT_ACCOUNT.errors) to the component's state. This meant updating the functions in ReimbursementAccountUtils.js that helped in reusing code for our error handling. If these changes make sense for the people reviewing this, I would say this pull request is pretty much ready :) |
@@ -206,7 +206,7 @@ function isValidURL(url) { | |||
* @returns {Object} | |||
*/ | |||
function validateIdentity(identity) { | |||
const requiredFields = ['firstName', 'lastName', 'street', 'city', 'zipCode', 'state', 'ssnLast4', 'dob']; | |||
const requiredFields = ['firstName', 'lastName', 'addressStreet', 'addressCity', 'addressZipCode', 'addressState', 'ssnLast4', 'dob']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed street
, city
, zipCode
, state
to addressStreet
, addressCity
, addressZipCode
, addressState
because IdentityForm
was using both ways of referring to these inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow the reasoning. Did it not work before? What is the advantage of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it for consistency, to avoid having something like this:
App/src/pages/ReimbursementAccount/IdentityForm.js
Lines 141 to 142 in 2aaa8fd
value={props.values.street} | |
onChangeText={value => props.onFieldChange('addressStreet', value)} |
Where we feed the input with city
, but then we call onFieldChange('addressCity', value)
.
@@ -1,7 +1,6 @@ | |||
import lodashGet from 'lodash/get'; | |||
import lodashUnset from 'lodash/unset'; | |||
import lodashCloneDeep from 'lodash/cloneDeep'; | |||
import {setBankAccountFormValidationErrors} from './actions/BankAccounts'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the functions in this file to work with the errors being stored in the component's state rather than in Onyx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB, seems like this could have been moved into a separate PR.
Update: resolve conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. I have two comments:
- We should apply the same changes to AddDebitCardPage.
- I'm not a big fan of how we enter unit/apt numbers. The search dropdown opens with different results and we have to click away from it to close the dropdown. Can we prevent it from suggesting results if we already have selected one, but suggest again if we clear the field?
search.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to test well, had a few implementation questions
addressState, | ||
addressZipCode, | ||
addressStreet, | ||
}, _.identity)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to pass the key and value for all of these properties to props.onChange
. What's the value of doing it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The object passed to props.onChange
will optionally have the following keys: addressCity
, addressState
, addressZipCode
and addressStreet
. The _pick(..., _.identity)
will remove any of they keys that don't have a truthy
value associated. Why? because if the clicked result is missing some part of the address (i.e. city), we are not going to overwrite what the user may have typed.
Now that you mentioned, the intention is not obvious by looking at the code.
I could add a comment to make it more clear, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a comment would be very helpful in this case, I missed the overwriting part but that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB, maybe would be better without the underscore identity
trick and check for each value
const values = {};
if (addressCity) {
values.addressCity = addressCity;
}
// etc
if (_.size(values) === 0) {
return;
}
this.props.onChange(values);
<GooglePlacesAutocomplete | ||
fetchDetails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nab: my IDE is complaining about no placeholder
required attribute set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, there is a mandatory prop "placeholder" in GooglePlacesAutocomplete
(see here). We haven't been using it.
If I add placeholder="PLACEHOLDER"
, it looks like this:
Screen.Recording.2021-11-22.at.11.00.36.AM.mov
We could get rid of the error by just doing placeholder=""
if we don't want to have a placeholder text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was going to suggest just doing placeholder=""
, I think that'd be best
I'll have a look
Agree on that!
Sounds complicated to implement properly, like you would have to keep some state to signal that it was cleared and that we now allow results again. Another option would be to read if the user typed a I think that the best would be a separate Apt field, but then there is the problem that we currently handle it as one field in the backend. Maybe an easy approach would be to:
This would allow the rest of the code using our @kevinksullivan shared this from another app which in my opinion is what works best (separate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a 50% review. I think we should maybe separate out the work of moving errors to it's own PR - it's making it a bit hard to review the changes to the address component and I'm not sure what they have to do with eachother.
this.clearError = inputKey => ReimbursementAccountUtils.clearError(this.props, inputKey); | ||
this.clearError = ReimbursementAccountUtils.clearError.bind(this); | ||
this.getErrors = ReimbursementAccountUtils.getErrors.bind(this); | ||
this.setErrors = ReimbursementAccountUtils.setErrors.bind(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB, personally, I think it's fine to explicitly pass the props rather than implicitly bind to this
. Perhaps it would be better to not spend time improving things we know we are going to soon replace and only change what we need to accomplish the task at hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but I changed this because I started using this.state
and this.setState
in clearError
. Didn't check this properly, but I thought that passing this.setState
would already require a binding so I thought it was just better to bind clearError
. (#6200 (comment))
googlePlacesRef.current.setAddressText(props.value); | ||
}, []); | ||
this.state = { | ||
skippedFirstOnChangeText: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the ref was more intuitive than this and would recommend we go back to that if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, I don't think it makes it more intuitive, but I'm ok with changing it. If I change it to ref, this would mean that this component should be a functional component again, right?
|
||
const saveLocationDetails = (details) => { | ||
saveLocationDetails(details) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a doc
const addressComponents = details.address_components; | ||
if (isAddressValidForVBA(addressComponents)) { | ||
if (addressComponents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!addressComponents) {
return;
}
addressState, | ||
addressZipCode, | ||
addressStreet, | ||
}, _.identity)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB, maybe would be better without the underscore identity
trick and check for each value
const values = {};
if (addressCity) {
values.addressCity = addressCity;
}
// etc
if (_.size(values) === 0) {
return;
}
this.props.onChange(values);
value: this.props.value, | ||
onChangeText: (text) => { | ||
// Line of code https://github.com/FaridSafi/react-native-google-places-autocomplete/blob/47d7223dd48f85da97e80a0729a985bbbcee353f/GooglePlacesAutocomplete.js#L148 | ||
// will call onChangeText passing '' after the component renders the first time, clearing its value. Why? who knows, but we have to skip it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just me, but I think we should either figure out why it happens or just say we are working around a feature of the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But ideally we don't do this at all and there is no explanation necessary 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess it is not the best comment. The point is, I know why it happens (like what code in the library is creating the problem), but I don't know the reason why they decided to do that (doesn't make sense to me).
we are working around a feature of the library.
This ^
@@ -1,7 +1,6 @@ | |||
import lodashGet from 'lodash/get'; | |||
import lodashUnset from 'lodash/unset'; | |||
import lodashCloneDeep from 'lodash/cloneDeep'; | |||
import {setBankAccountFormValidationErrors} from './actions/BankAccounts'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB, seems like this could have been moved into a separate PR.
@@ -18,44 +17,55 @@ function getDefaultStateForField(props, fieldName, defaultValue = '') { | |||
} | |||
|
|||
/** | |||
* @param {Object} props | |||
* Use this function binding it to a component's instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we have made this method less flexible and harder to test since it now requires a "component instance" to be bound to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I thought I could be making it harder to test, but then I thought that in a test you would have to bind it to an object, not necessarily a component and that shouldn't be too complicated.
When I changed the errors to be stored in the state of the component, I started needing access to state
and setState
instead of just props
. I thought that to pass setState
I would need to bind setState
to this
anyway before passing it, so it made some sense to me to make these functions "binded" for easier use in the components. Not all of the modified functions benefited to be like this, but I changed them all in the same way to make them more consistent.
@@ -206,7 +206,7 @@ function isValidURL(url) { | |||
* @returns {Object} | |||
*/ | |||
function validateIdentity(identity) { | |||
const requiredFields = ['firstName', 'lastName', 'street', 'city', 'zipCode', 'state', 'ssnLast4', 'dob']; | |||
const requiredFields = ['firstName', 'lastName', 'addressStreet', 'addressCity', 'addressZipCode', 'addressState', 'ssnLast4', 'dob']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow the reasoning. Did it not work before? What is the advantage of this change?
I'm ok with that solution. How would we handle displaying server data though? |
The changes in the address component started causing the problem described here: #6200 (comment) I thought about doing a separate PR for moving errors to the component's state and have more readable PR's, but then this PR would have had the problem described. I could redo and separate these things if it makes sense for you. This would make sense to me:
|
Update: Redid this PR here reducing the scope. It is not moving errors from Onyx to the form state anymore, hopefully it will make it easier to review :) |
Sorry for the long PR
Pullerbear: nickmurray47
Details
state
,city
andzipCode
fields back in the following steps: "Company Step", "Personal information (requestor)" and "Additional information (beneficial owners)"addressStreet
if thestreet name + street number
coming from the google places result is longer than what the user typed.googlePlacesRef
as I don't see the point of having that anymore. We can just pass the value down that is coming from the form astextInputProps
skippedFirstOnChangeText
state to skip the first time the library calls our text input'sonChangeText
callback. I'm not sure why (doesn't make sense to me), the library callsonChangeText
with an empty string""
to clear the value. Clearing the value in our case is not something we want when we are returning to a step that we already filled.IdentityForm
was reading address inputs using keyscity
,zipCode
andstate
, but it was callingonFieldChange
usingaddressCity
,addressZipCode
andaddressState
. Made it more consistent and changed it to always useaddressCity
,addressZipCode
andaddressState
.AddressSearch
to class componentFixed Issues
$ #6309
$ https://github.com/Expensify/Expensify/issues/181598
$ #5841
Tests / QA steps
Company address
,City
,Zip Code
, andState
:Company address
and select a result. The other address fields (City
,Zip Code
, andState
) should get updated with the information from the selected result.Company address
field and type#12334
at the end (adding the apt number).Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android