-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Show recent waypoints when selecting address for distance request #25974
Changes from 4 commits
c4f93a3
c50feeb
f9dcecf
f58407a
2c9ffa5
e791e02
b818dba
ff8e227
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 |
---|---|---|
|
@@ -61,6 +61,26 @@ const propTypes = { | |
/** Should address search be limited to results in the USA */ | ||
isLimitedToUSA: PropTypes.bool, | ||
|
||
/** A list of predefined places that can be shown when the user isn't searching for something */ | ||
predefinedPlaces: PropTypes.arrayOf( | ||
PropTypes.shape({ | ||
/** A description of the location (usually the address) */ | ||
description: PropTypes.string, | ||
|
||
/** Data required by the google auto complete plugin to know where to put the markers on the map */ | ||
geometry: PropTypes.shape({ | ||
/** Data about the location */ | ||
location: PropTypes.shape({ | ||
/** Lattitude of the location */ | ||
lat: PropTypes.number, | ||
|
||
/** Longitude of the location */ | ||
lng: PropTypes.number, | ||
}), | ||
}), | ||
}), | ||
), | ||
|
||
/** A map of inputID key names */ | ||
renamedInputKeys: PropTypes.shape({ | ||
street: PropTypes.string, | ||
|
@@ -102,6 +122,7 @@ const defaultProps = { | |
lng: 'addressLng', | ||
}, | ||
maxInputLength: undefined, | ||
predefinedPlaces: [], | ||
}; | ||
|
||
// Do not convert to class component! It's been tried before and presents more challenges than it's worth. | ||
|
@@ -122,6 +143,19 @@ function AddressSearch(props) { | |
const saveLocationDetails = (autocompleteData, details) => { | ||
const addressComponents = details.address_components; | ||
if (!addressComponents) { | ||
// When there are details, but no address_components, this indicates that some predefined options have been passed | ||
// to this component which don't match the usual properties coming from auto-complete. In that case, only a limited | ||
// amount of data massaging needs to happen for what the parent expects to get from this function. | ||
if (_.size(details)) { | ||
props.onPress({ | ||
address: lodashGet(details, 'description', ''), | ||
lat: lodashGet(details, 'geometry.location.lat', 0), | ||
lng: lodashGet(details, 'geometry.location.lng', 0), | ||
}); | ||
|
||
// After we select an option, we set displayListViewBorder to false to prevent UI flickering | ||
setDisplayListViewBorder(false); | ||
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. This should be outside the if condition. I think we should actually revert the change related to this line. It's better to just call this function on the 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. Sure, I can move it back outside. It made more sense to move it inside before I added this block and had to repeat calling |
||
} | ||
return; | ||
} | ||
|
||
|
@@ -223,6 +257,9 @@ function AddressSearch(props) { | |
} | ||
|
||
props.onPress(values); | ||
|
||
// After we select an option, we set displayListViewBorder to false to prevent UI flickering | ||
setDisplayListViewBorder(false); | ||
}; | ||
|
||
return ( | ||
|
@@ -250,17 +287,13 @@ function AddressSearch(props) { | |
fetchDetails | ||
suppressDefaultStyles | ||
enablePoweredByContainer={false} | ||
predefinedPlaces={props.predefinedPlaces} | ||
ListEmptyComponent={ | ||
props.network.isOffline ? null : ( | ||
<Text style={[styles.textLabel, styles.colorMuted, styles.pv4, styles.ph3, styles.overflowAuto]}>{props.translate('common.noResultsFound')}</Text> | ||
) | ||
} | ||
onPress={(data, details) => { | ||
saveLocationDetails(data, details); | ||
|
||
// After we select an option, we set displayListViewBorder to false to prevent UI flickering | ||
setDisplayListViewBorder(false); | ||
}} | ||
onPress={saveLocationDetails} | ||
query={query} | ||
requestUrl={{ | ||
useOnPlatform: 'all', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,7 +53,7 @@ | |
transaction: {}, | ||
}; | ||
|
||
function WaypointEditor({transactionID, route: {params: {iouType = '', waypointIndex = ''} = {}} = {}, network, translate, transaction}) { | ||
function WaypointEditor({transactionID, route: {params: {iouType = '', waypointIndex = ''} = {}} = {}, network, translate, transaction, recentWaypoints}) { | ||
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. 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. wonder why this wasn't caught during PR CI checks 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. Yes. It got caught in our PR but not here 🤔 🤔 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. this is handled by @allroundexperts now 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. Oh wow, yes... I'm surprised I missed it and doubly surprised that the linter missed it! Thanks for handling this. |
||
const textInput = useRef(null); | ||
const currentWaypoint = lodashGet(transaction, `comment.waypoints.waypoint${waypointIndex}`, {}); | ||
const waypointAddress = lodashGet(currentWaypoint, 'address', ''); | ||
|
@@ -151,6 +151,7 @@ | |
lng: null, | ||
state: null, | ||
}} | ||
predefinedPlaces={recentWaypoints} | ||
/> | ||
</View> | ||
</Form> | ||
|
@@ -169,5 +170,22 @@ | |
key: (props) => `${ONYXKEYS.COLLECTION.TRANSACTION}${props.transactionID}`, | ||
selector: (transaction) => (transaction ? {transactionID: transaction.transactionID, comment: {waypoints: lodashGet(transaction, 'comment.waypoints')}} : null), | ||
}, | ||
|
||
recentWaypoints: { | ||
key: ONYXKEYS.NVP_RECENT_WAYPOINTS, | ||
|
||
// Only grab the most recent 5 waypoints because that's all that is shown in the UI. This also puts them into the format of data | ||
// that the google autocomplete component expects for it's "predefined places" feature. | ||
selector: (waypoints) => | ||
_.map(waypoints && waypoints.length ? waypoints.slice(0, 5) : [], (waypoint) => ({ | ||
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. Lint. Underscore is not imported 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. NAB. Just curious why we check for `waypoints.length? 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. I think I was just being paranoid about calling |
||
description: waypoint.address, | ||
geometry: { | ||
location: { | ||
lat: waypoint.lat, | ||
lng: waypoint.lng, | ||
}, | ||
}, | ||
})), | ||
}, | ||
}), | ||
)(WaypointEditor); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
type RecentWaypoints = { | ||
/** The full address of the waypoint */ | ||
address: string; | ||
|
||
/** The lattitude of the waypoint */ | ||
lat: number; | ||
|
||
/** The longitude of the waypoint */ | ||
lng: number; | ||
}; | ||
|
||
export default RecentWaypoints; |
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. For consistency between inputs and outputs, I think we should use the waypoint shape here:
address
,lat
and `lng. The GooglePlacesAutocomplete is used internally, the props shape conversion should be done inside and not outside. i.e. the Onyx selector should not change the shape of the object.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.
Thanks, I understand your point. This is why I did not go that route (but I did consider it).
<AddressSearch>
component is used in many places.<AddressSearch>
would only be used for ONE of the places the component is implemented and won't be used for any of the other places. This is a waste.<AddressSearch>
makes it very highly coupled to the parent component (because it is assuming knowledge about the data being passed into it). This makes the component harder to reusepredefinedPlaces
prop with this shape allows any other component to use the same feature and not have to worry about conforming to the "waypoint shape".