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

Enable again all address fields and allow the user to manually fill them / Move form errors to component state #6200

Closed
wants to merge 13 commits into from

Conversation

aldo-expensify
Copy link
Contributor

@aldo-expensify aldo-expensify commented Nov 4, 2021

Sorry for the long PR

Pullerbear: nickmurray47

Details

  • Added state, city and zipCode fields back in the following steps: "Company Step", "Personal information (requestor)" and "Additional information (beneficial owners)"
  • When we select a result from the address autocomplete, we use the information that is available. If some part is missing (i.e. city), we just ignore that.
  • We only replace the addressStreet if the street name + street number coming from the google places result is longer than what the user typed.
  • Removed 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 as textInputProps
  • Introduced skippedFirstOnChangeText state to skip the first time the library calls our text input's onChangeText callback. I'm not sure why (doesn't make sense to me), the library calls onChangeText 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.
  • Deleted unused validation method for Google places address components
  • IdentityForm was reading address inputs using keys city, zipCode and state, but it was calling onFieldChange using addressCity, addressZipCode and addressState. Made it more consistent and changed it to always use addressCity, addressZipCode and addressState.
  • Converted AddressSearch to class component
  • Moved the form validation errors from Onyx to the form component state because of problem explained here

Fixed Issues

$ #6309
$ https://github.com/Expensify/Expensify/issues/181598
$ #5841

Tests / QA steps

  1. Create NewDot account, verify it.
  2. Create workspace and start the add VBA flow
  3. In "Company information" step:
    1. You should see the inputs: Company address, City, Zip Code, and State:
    2. Click "Save & Continue", they should get inline errors
    3. Typing in any of the field should clear the error.
    4. Type and address in Company address and select a result. The other address fields (City, Zip Code, and State) should get updated with the information from the selected result.
    5. Focus on the Company address field and type #12334 at the end (adding the apt number).
    6. Click "Save & Continue", you should be able to proceed to the next step.
  4. Do the same steps (sub-steps in step (3)) for "Personal information"
  5. Do the same steps (sub-steps in step (3)) for "Additional information" adding beneficial owners

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@Jag96 Jag96 self-requested a review November 4, 2021 00:55
@aldo-expensify aldo-expensify self-assigned this Nov 4, 2021
Copy link
Contributor

@Jag96 Jag96 left a 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.


googlePlacesRef.current.setAddressText(props.value);
}, []);
const [skippedFirstOnChangeText, setSkippedFirstOnChangeText] = useState(false);
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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 😂

Copy link
Contributor Author

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.

Copy link
Contributor

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.

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.
Copy link
Contributor

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?

Copy link
Contributor Author

@aldo-expensify aldo-expensify Nov 5, 2021

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?
Copy link
Contributor

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.

Copy link
Contributor Author

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!

Copy link
Contributor

@marcaaron marcaaron left a 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".

props.onChangeText('addressState', state);
}
if (zipCode) {
props.onChangeText('addressZipCode', zipCode);
Copy link
Contributor

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});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree with this

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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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 Show resolved Hide resolved

googlePlacesRef.current.setAddressText(props.value);
}, []);
const [skippedFirstOnChangeText, setSkippedFirstOnChangeText] = useState(false);
Copy link
Contributor

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.

@luacmartins luacmartins self-requested a review November 8, 2021 17:22
Copy link
Contributor

@luacmartins luacmartins left a 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.

@aldo-expensify
Copy link
Contributor Author

Update:

  • Renamed AddressSearch onTextChange prop to onChange.
  • AddressSearch now passes and object with the keys: addressStreet, addressCity,addressState, addressZipCode to the onChange callback
  • Update IdentityForm and Company step form to work with the changes of AddressSearch

New problem

Because we are using separate validation again for the fields addressStreet, addressCity,addressState, addressZipCode, and when a user clicks a result we clear all of them one by one, we end up clearing the errors incorrectly:

Screen.Recording.2021-11-08.at.5.27.25.PM.mov

The 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 Onyx and keep it at the level of the form (step) in a react state. There isn't a big reason to keep it in Onyx if we are not interested in it persisting on reload.

@aldo-expensify
Copy link
Contributor Author

aldo-expensify commented Nov 8, 2021

I just noticed that we got some duplicated work going for different solutions for the same problem:

PR: #6210

GH issues:

Expensify/Expensify#183067
Expensify/Expensify#183908

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?

@kevinksullivan
Copy link
Contributor

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.

@Jag96
Copy link
Contributor

Jag96 commented Nov 9, 2021

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.

@aldo-expensify
Copy link
Contributor Author

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).
@aldo-expensify
Copy link
Contributor Author

aldo-expensify commented Nov 18, 2021

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'];
Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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:

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';
Copy link
Contributor Author

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.

Copy link
Contributor

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.

@aldo-expensify
Copy link
Contributor Author

Update: resolve conflicts

@aldo-expensify aldo-expensify marked this pull request as ready for review November 18, 2021 20:21
@aldo-expensify aldo-expensify requested a review from a team as a code owner November 18, 2021 20:21
@aldo-expensify aldo-expensify changed the title [WIP] Enable again all address fields and allow the user to manually fill them Enable again all address fields and allow the user to manually fill them / Move form errors to component state Nov 18, 2021
@MelvinBot MelvinBot requested review from nickmurray47 and removed request for a team November 18, 2021 20:21
Copy link
Contributor

@luacmartins luacmartins left a 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:

  1. We should apply the same changes to AddDebitCardPage.
  2. 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

Copy link
Contributor

@nickmurray47 nickmurray47 left a 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

src/components/AddressSearch.js Outdated Show resolved Hide resolved
addressState,
addressZipCode,
addressStreet,
}, _.identity));
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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);

Comment on lines +77 to +78
<GooglePlacesAutocomplete
fetchDetails
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

src/libs/GooglePlacesUtils.js Show resolved Hide resolved
@aldo-expensify
Copy link
Contributor Author

aldo-expensify commented Nov 22, 2021

Code LGTM. I have two comments:

  1. We should apply the same changes to AddDebitCardPage.

I'll have a look

  1. 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.

Agree on that!

Can we prevent it from suggesting results if we already have selected one, but suggest again if we clear the field?

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 # near the end to detect that is typing an apartment, but I don't think it would cover all cases (some users may type Apt 123).

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:

  • Add Apt input in the form
  • Store street number + street name in a new key (i.e. streetNameAndNumber) without apt number
  • Store apt number in a new key (i.e. aptNumber)
  • Store them together in the key we are already using (redundant)

This would allow the rest of the code using our street name + number + apt number to keep working, but then on the form, we show the data from the two new keys separate. Each time we change either aptNumber or streetNameAndNumber, we would have to take care of updating the redundant concatenated info.

@kevinksullivan shared this from another app which in my opinion is what works best (separate apt input):
image
image

Copy link
Contributor

@marcaaron marcaaron left a 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);
Copy link
Contributor

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.

Copy link
Contributor Author

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,
Copy link
Contributor

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.

Copy link
Contributor Author

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) {
Copy link
Contributor

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) {
Copy link
Contributor

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));
Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor

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 😄

Copy link
Contributor Author

@aldo-expensify aldo-expensify Nov 26, 2021

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';
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

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'];
Copy link
Contributor

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?

@luacmartins
Copy link
Contributor

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 # near the end to detect that is typing an apartment, but I don't think it would cover all cases (some users may type Apt 123).

I'm ok with that solution. How would we handle displaying server data though?

@aldo-expensify
Copy link
Contributor Author

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.

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:

  • PR 1: Move errors to components state (should have not visible effect in the UI)
  • PR 2 [HOLD PR 1]: Do the changes related to address component.

@aldo-expensify
Copy link
Contributor Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants