-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Update Address search - show all inputs #6569
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
Changes from all commits
2865a78
8f8d148
32a9eb9
91920d6
bf63468
0ea709a
9295c47
27b0d84
3f73b08
5adf0c2
7d44cd8
d7ac198
93642a8
0865523
b0fe744
f8377ad
aed7b8c
e7ff88a
fd5316e
3fd626a
c30f08a
ec758cc
8466121
3cddfcf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import _ from 'underscore'; | ||
| import React, {useEffect, useState, useRef} from 'react'; | ||
| import React, {useRef, useState} from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import {LogBox} from 'react-native'; | ||
| import {GooglePlacesAutocomplete} from 'react-native-google-places-autocomplete'; | ||
|
|
@@ -23,7 +23,7 @@ const propTypes = { | |
| value: PropTypes.string, | ||
|
|
||
| /** A callback function when the value of this field has changed */ | ||
| onChangeText: PropTypes.func.isRequired, | ||
| onChange: PropTypes.func.isRequired, | ||
|
|
||
| /** Customize the ExpensiTextInput container */ | ||
| containerStyles: PropTypes.arrayOf(PropTypes.object), | ||
|
|
@@ -40,52 +40,53 @@ const defaultProps = { | |
| // Relevant thread: https://expensify.slack.com/archives/C03TQ48KC/p1634088400387400 | ||
| // Reference: https://github.com/FaridSafi/react-native-google-places-autocomplete/issues/609#issuecomment-886133839 | ||
| const AddressSearch = (props) => { | ||
| const googlePlacesRef = useRef(); | ||
| const [displayListViewBorder, setDisplayListViewBorder] = useState(false); | ||
| useEffect(() => { | ||
| if (!googlePlacesRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| googlePlacesRef.current.setAddressText(props.value); | ||
| }, []); | ||
| // We use `skippedFirstOnChangeTextRef` to work around a feature of the library: | ||
| // The library is calling onChangeText with '' at the start and we don't need this | ||
| // https://github.com/FaridSafi/react-native-google-places-autocomplete/blob/47d7223dd48f85da97e80a0729a985bbbcee353f/GooglePlacesAutocomplete.js#L148 | ||
| const skippedFirstOnChangeTextRef = useRef(false); | ||
|
|
||
| const saveLocationDetails = (details) => { | ||
| const addressComponents = details.address_components; | ||
| if (GooglePlacesUtils.isAddressValidForVBA(addressComponents)) { | ||
| // Gather the values from the Google details | ||
| const streetNumber = GooglePlacesUtils.getAddressComponent(addressComponents, 'street_number', 'long_name'); | ||
| const streetName = GooglePlacesUtils.getAddressComponent(addressComponents, 'route', 'long_name'); | ||
| let city = GooglePlacesUtils.getAddressComponent(addressComponents, 'locality', 'long_name'); | ||
| if (!city) { | ||
| city = GooglePlacesUtils.getAddressComponent(addressComponents, 'sublocality', 'long_name'); | ||
| Log.hmmm('[AddressSearch] Replacing missing locality with sublocality: ', {address: details.formatted_address, sublocality: city}); | ||
| } | ||
| const state = GooglePlacesUtils.getAddressComponent(addressComponents, 'administrative_area_level_1', 'short_name'); | ||
| const zipCode = GooglePlacesUtils.getAddressComponent(addressComponents, 'postal_code', 'long_name'); | ||
|
|
||
| // Trigger text change events for each of the individual fields being saved on the server | ||
| props.onChangeText('addressStreet', `${streetNumber} ${streetName}`); | ||
| props.onChangeText('addressCity', city); | ||
| props.onChangeText('addressState', state); | ||
| props.onChangeText('addressZipCode', zipCode); | ||
| } else { | ||
| // Clear the values associated to the address, so our validations catch the problem | ||
| Log.hmmm('[AddressSearch] Search result failed validation: ', { | ||
| address: details.formatted_address, | ||
| address_components: addressComponents, | ||
| place_id: details.place_id, | ||
| }); | ||
| props.onChangeText('addressStreet', null); | ||
| props.onChangeText('addressCity', null); | ||
| props.onChangeText('addressState', null); | ||
| props.onChangeText('addressZipCode', null); | ||
| if (!addressComponents) { | ||
| return; | ||
| } | ||
|
|
||
| // Gather the values from the Google details | ||
| const streetNumber = GooglePlacesUtils.getAddressComponent(addressComponents, 'street_number', 'long_name') || ''; | ||
| const streetName = GooglePlacesUtils.getAddressComponent(addressComponents, 'route', 'long_name') || ''; | ||
| const street = `${streetNumber} ${streetName}`.trim(); | ||
| let city = GooglePlacesUtils.getAddressComponent(addressComponents, 'locality', 'long_name'); | ||
| if (!city) { | ||
| city = GooglePlacesUtils.getAddressComponent(addressComponents, 'sublocality', 'long_name'); | ||
| Log.hmmm('[AddressSearch] Replacing missing locality with sublocality: ', {address: details.formatted_address, sublocality: city}); | ||
| } | ||
| const zipCode = GooglePlacesUtils.getAddressComponent(addressComponents, 'postal_code', 'long_name'); | ||
| const state = GooglePlacesUtils.getAddressComponent(addressComponents, 'administrative_area_level_1', 'short_name'); | ||
|
|
||
| const values = {}; | ||
| if (street && street.length > props.value.length) { | ||
| // Don't replace if the user has typed something longer. I.e. maybe the user entered the Apt # | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the change! NAB, this comment does help explain why we might not want to use the street number and street name provided by Google Places. I do have a lingering concern that we are not quite painting the exact picture of why we need this logic. Here's a suggestion that is more verbose, but explains the situation in greater detail
|
||
| values.street = street; | ||
| } | ||
| if (city) { | ||
| values.city = city; | ||
| } | ||
| if (zipCode) { | ||
| values.zipCode = zipCode; | ||
| } | ||
| if (state) { | ||
| values.state = state; | ||
| } | ||
| if (_.size(values) === 0) { | ||
| return; | ||
| } | ||
| props.onChange(values); | ||
| }; | ||
|
|
||
| return ( | ||
| <GooglePlacesAutocomplete | ||
| ref={googlePlacesRef} | ||
| fetchDetails | ||
| suppressDefaultStyles | ||
| enablePoweredByContainer={false} | ||
|
|
@@ -110,12 +111,12 @@ const AddressSearch = (props) => { | |
| label: props.label, | ||
| containerStyles: props.containerStyles, | ||
| errorText: props.errorText, | ||
| value: props.value, | ||
|
marcaaron marked this conversation as resolved.
|
||
| onChangeText: (text) => { | ||
| const isTextValid = !_.isEmpty(text) && _.isEqual(text, props.value); | ||
|
|
||
| // Ensure whether an address is selected already or has address value initialized. | ||
| if (!_.isEmpty(googlePlacesRef.current.getAddressText()) && !isTextValid) { | ||
| saveLocationDetails({}); | ||
| if (skippedFirstOnChangeTextRef.current) { | ||
| props.onChange({street: text}); | ||
| } else { | ||
| skippedFirstOnChangeTextRef.current = true; | ||
| } | ||
|
|
||
| // If the text is empty, we set displayListViewBorder to false to prevent UI flickering | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| import React from 'react'; | ||
| import {View} from 'react-native'; | ||
| import PropTypes from 'prop-types'; | ||
| import ExpensiTextInput from '../../components/ExpensiTextInput'; | ||
| import AddressSearch from '../../components/AddressSearch'; | ||
| import styles from '../../styles/styles'; | ||
| import withLocalize, {withLocalizePropTypes} from '../../components/withLocalize'; | ||
| import CONST from '../../CONST'; | ||
| import StatePicker from '../../components/StatePicker'; | ||
| import Text from '../../components/Text'; | ||
|
|
||
| const propTypes = { | ||
| /** Callback fired when a field changes. Passes args as {[fieldName]: val} */ | ||
| onFieldChange: PropTypes.func.isRequired, | ||
|
|
||
| /** Form values */ | ||
| values: PropTypes.shape({ | ||
| /** Address street field */ | ||
| street: PropTypes.string, | ||
|
|
||
| /** Address city field */ | ||
| city: PropTypes.string, | ||
|
|
||
| /** Address state field */ | ||
| state: PropTypes.string, | ||
|
|
||
| /** Address zip code field */ | ||
| zipCode: PropTypes.string, | ||
| }), | ||
|
|
||
| /** Any errors that can arise from form validation */ | ||
| errors: PropTypes.objectOf(PropTypes.bool), | ||
|
|
||
| ...withLocalizePropTypes, | ||
| }; | ||
|
|
||
| const defaultProps = { | ||
| values: { | ||
| street: '', | ||
| city: '', | ||
| state: '', | ||
| zipCode: '', | ||
| }, | ||
| errors: {}, | ||
| }; | ||
|
|
||
| const AddressForm = props => ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for refactoring this! |
||
| <> | ||
| <AddressSearch | ||
| label={props.translate('common.personalAddress')} | ||
| containerStyles={[styles.mt4]} | ||
| value={props.values.street} | ||
| onChange={props.onFieldChange} | ||
| errorText={props.errors.street ? props.translate('bankAccount.error.addressStreet') : ''} | ||
| /> | ||
| <Text style={[styles.mutedTextLabel, styles.mt1]}>{props.translate('common.noPO')}</Text> | ||
| <View style={[styles.flexRow, styles.mt4]}> | ||
| <View style={[styles.flex2, styles.mr2]}> | ||
| <ExpensiTextInput | ||
| label={props.translate('common.city')} | ||
| value={props.values.city} | ||
| onChangeText={value => props.onFieldChange({city: value})} | ||
| errorText={props.errors.city ? props.translate('bankAccount.error.addressCity') : ''} | ||
| /> | ||
| </View> | ||
| <View style={[styles.flex1]}> | ||
| <StatePicker | ||
| value={props.values.state} | ||
| onChange={value => props.onFieldChange({state: value})} | ||
| errorText={props.errors.state ? props.translate('bankAccount.error.addressState') : ''} | ||
| hasError={Boolean(props.errors.state)} | ||
| /> | ||
| </View> | ||
| </View> | ||
| <ExpensiTextInput | ||
| label={props.translate('common.zip')} | ||
| containerStyles={[styles.mt4]} | ||
| keyboardType={CONST.KEYBOARD_TYPE.NUMERIC} | ||
| value={props.values.zipCode} | ||
| onChangeText={value => props.onFieldChange({zipCode: value})} | ||
| errorText={props.errors.zipCode ? props.translate('bankAccount.error.zipCode') : ''} | ||
| maxLength={CONST.BANK_ACCOUNT.MAX_LENGTH.ZIP_CODE} | ||
| /> | ||
| </> | ||
| ); | ||
|
|
||
| AddressForm.propTypes = propTypes; | ||
| AddressForm.defaultProps = defaultProps; | ||
| AddressForm.displayName = 'AddressForm'; | ||
| export default withLocalize(AddressForm); | ||
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, should we trim
props.value.lengthin case there is whitespace that makes it longer but less accurate?Also, I wonder if we need to worry about a case like this where someone enters some very long address into the street input. I feel like in trying to optimize for the Apartment # case we are maybe introducing the possibility of bad street info instead of trusting the correctness of what Google Places returns.
Here's an example of what I mean:
I think we can maybe wait and see if this is a problem...
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 honestly don't like it either, I think the ideal case is to have the Apt number in a separate field. As @luacmartins , this also causes the autocomplete to start showing results while the user may be only trying to input the apt number and it looks weird :P. Having said that, if we wanted to do a separate input for it, I think that should be done in a separate pr/issue.