Enable again all address fields and allow the user to manually fill them / Move form errors to component state #6200
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import React, {PureComponent} from 'react'; | ||
| import _ from 'underscore'; | ||
| import PropTypes from 'prop-types'; | ||
| import {LogBox} from 'react-native'; | ||
| import {GooglePlacesAutocomplete} from 'react-native-google-places-autocomplete'; | ||
|
|
@@ -8,7 +8,7 @@ import withLocalize, {withLocalizePropTypes} from './withLocalize'; | |
| import styles from '../styles/styles'; | ||
| import ExpensiTextInput from './ExpensiTextInput'; | ||
| import Log from '../libs/Log'; | ||
| import {getAddressComponent} from '../libs/GooglePlacesUtils'; | ||
|
|
||
| // The error that's being thrown below will be ignored until we fork the | ||
| // react-native-google-places-autocomplete repo and replace the | ||
|
|
@@ -23,134 +23,127 @@ const propTypes = { | |
| value: PropTypes.string, | ||
|
|
||
| /** A callback function when the value of this field has changed */ | ||
| onChange: PropTypes.func.isRequired, | ||
|
|
||
| /** Customize the ExpensiTextInput container */ | ||
| containerStyles: PropTypes.arrayOf(PropTypes.object), | ||
|
|
||
| ...withLocalizePropTypes, | ||
| }; | ||
|
|
||
| const defaultProps = { | ||
| value: '', | ||
| containerStyles: null, | ||
| }; | ||
|
|
||
| class AddressSearch extends PureComponent { | ||
| constructor(props) { | ||
| super(props); | ||
|
|
||
| this.state = { | ||
| skippedFirstOnChangeText: false, | ||
|
Contributor
There was a problem hiding this comment. Having the ref was more intuitive than this and would recommend we go back to that if possible.
Contributor
Author
There was a problem hiding this comment. 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? |
||
| displayListViewBorder: false, | ||
| }; | ||
| } | ||
|
|
||
| saveLocationDetails(details) { | ||
|
|
||
| const addressComponents = details.address_components; | ||
marcaaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (addressComponents) { | ||
|
Contributor
There was a problem hiding this comment. if (!addressComponents) {
return;
} |
||
| // Gather the values from the Google details | ||
| const streetNumber = getAddressComponent(addressComponents, 'street_number', 'long_name') || ''; | ||
| const streetName = getAddressComponent(addressComponents, 'route', 'long_name') || ''; | ||
|
|
||
| const addressStreet = `${streetNumber} ${streetName}`.trim(); | ||
|
|
||
aldo-expensify marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| let addressCity = getAddressComponent(addressComponents, 'locality', 'long_name'); | ||
| if (!addressCity) { | ||
| addressCity = getAddressComponent(addressComponents, 'sublocality', 'long_name'); | ||
| Log.hmmm('[AddressSearch] Replacing missing locality with sublocality: ', {address: details.formatted_address, sublocality: addressCity}); | ||
| } | ||
| const addressZipCode = getAddressComponent(addressComponents, 'postal_code', 'long_name'); | ||
| const addressState = getAddressComponent(addressComponents, 'administrative_area_level_1', 'short_name'); | ||
|
|
||
| this.props.onChange(_.pick({ | ||
| addressCity, | ||
| addressState, | ||
| addressZipCode, | ||
| addressStreet, | ||
| }, _.identity)); | ||
|
Contributor
There was a problem hiding this comment. We are trying to pass the key and value for all of these properties to
Contributor
Author
There was a problem hiding this comment. The object passed to 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?
Contributor
There was a problem hiding this comment. Adding a comment would be very helpful in this case, I missed the overwriting part but that makes sense.
Contributor
There was a problem hiding this comment. NAB, maybe would be better without the underscore const values = {};
if (addressCity) {
values.addressCity = addressCity;
}
// etc
if (_.size(values) === 0) {
return;
}
this.props.onChange(values); |
||
| } | ||
| } | ||
|
|
||
| render() { | ||
| return ( | ||
| <GooglePlacesAutocomplete | ||
| fetchDetails | ||
|
Comment on lines
+75
to
+76
Contributor
There was a problem hiding this comment. nab: my IDE is complaining about no
Contributor
Author
There was a problem hiding this comment. You are right, there is a mandatory prop "placeholder" in If I add Screen.Recording.2021-11-22.at.11.00.36.AM.movWe could get rid of the error by just doing
Contributor
There was a problem hiding this comment. Was going to suggest just doing |
||
| suppressDefaultStyles | ||
| enablePoweredByContainer={false} | ||
| onPress={(data, details) => { | ||
| this.saveLocationDetails(details); | ||
|
|
||
| // After we select an option, we set displayListViewBorder to false to prevent UI flickering | ||
| this.setState({displayListViewBorder: false}); | ||
| }} | ||
| query={{ | ||
| key: 'AIzaSyC4axhhXtpiS-WozJEsmlL3Kg3kXucbZus', | ||
| language: this.props.preferredLocale, | ||
| types: 'address', | ||
| components: 'country:us', | ||
| }} | ||
| requestUrl={{ | ||
| useOnPlatform: 'web', | ||
| url: `${CONFIG.EXPENSIFY.URL_EXPENSIFY_COM}api?command=Proxy_GooglePlaces&proxyUrl=`, | ||
| }} | ||
| textInputProps={{ | ||
| InputComp: ExpensiTextInput, | ||
| label: this.props.label, | ||
| containerStyles: this.props.containerStyles, | ||
| errorText: this.props.errorText, | ||
| 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. | ||
|
Contributor
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. But ideally we don't do this at all and there is no explanation necessary 😄
Contributor
Author
There was a problem hiding this comment. 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).
This ^ |
||
| if (this.state.skippedFirstOnChangeText) { | ||
| this.props.onChange({addressStreet: text}); | ||
| } else { | ||
| this.setState({skippedFirstOnChangeText: true}); | ||
| } | ||
|
|
||
| // If the text is empty, we set displayListViewBorder to false to prevent UI flickering | ||
| if (_.isEmpty(text)) { | ||
| this.setState({displayListViewBorder: false}); | ||
| } | ||
| }, | ||
| }} | ||
| styles={{ | ||
| textInputContainer: [styles.flexColumn], | ||
| listView: [ | ||
| !this.state.displayListViewBorder && styles.googleListView, | ||
| this.state.displayListViewBorder && styles.borderTopRounded, | ||
| this.state.displayListViewBorder && styles.borderBottomRounded, | ||
| this.state.displayListViewBorder && styles.mt1, | ||
| styles.overflowAuto, | ||
| styles.borderLeft, | ||
| styles.borderRight, | ||
| ], | ||
| row: [ | ||
| styles.pv4, | ||
| styles.ph3, | ||
| styles.overflowAuto, | ||
| ], | ||
| description: [styles.googleSearchText], | ||
| separator: [styles.googleSearchSeparator], | ||
| }} | ||
| onLayout={(event) => { | ||
| // We use the height of the element to determine if we should hide the border of the listView dropdown | ||
| // to prevent a lingering border when there are no address suggestions. | ||
| // The height of the empty element is 2px (1px height for each top and bottom borders) | ||
| this.setState({displayListViewBorder: event.nativeEvent.layout.height > 2}); | ||
| }} | ||
| /> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| AddressSearch.propTypes = propTypes; | ||
| AddressSearch.defaultProps = defaultProps; | ||
|
|
||
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.stateandthis.setStateinclearError. Didn't check this properly, but I thought that passingthis.setStatewould already require a binding so I thought it was just better to bindclearError. (#6200 (comment))