Show recent waypoints when selecting address for distance request #25974
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
src/pages/iou/WaypointEditor.js
Outdated
| // 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.
Lint. Underscore is not imported
| /** 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, | ||
| }), | ||
| }), | ||
| }), | ||
| ), |
There was a problem hiding this comment.
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.
Thanks, I understand your point. This is why I did not go that route (but I did consider it).
- The
<AddressSearch>component is used in many places. - Adding logic to reformat this data inside of
<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. - Adding logic to reformat this data inside of
<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 reuse - Making the
predefinedPlacesprop with this shape allows any other component to use the same feature and not have to worry about conforming to the "waypoint shape".
| // After we select an option, we set displayListViewBorder to false to prevent UI flickering | ||
| setDisplayListViewBorder(false); |
There was a problem hiding this comment.
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 onPress callback once rather then calling it before every return statement in saveLocationDetails
There was a problem hiding this comment.
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 setDisplayListViewBorder in two locations.
src/pages/iou/WaypointEditor.js
Outdated
| // 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.
NAB. Just curious why we check for `waypoints.length?
There was a problem hiding this comment.
I think I was just being paranoid about calling .slice() on something that wasn't an array. It shouldn't ever be something other than an array, but you never know... I can remove it if you want.
src/libs/actions/Transaction.js
Outdated
| recentWaypoints.unshift(waypoint); | ||
| Onyx.merge(ONYXKEYS.NVP_RECENT_WAYPOINTS, recentWaypoints.slice(0, 5)); |
There was a problem hiding this comment.
Not a bug, but I think we shouldn't modify recentWaypoints
There was a problem hiding this comment.
That's a good point. I didn't think about this modifying the reference. I will change this.
|
Updated |
| const clonedWaypoints = _.clone(recentWaypoints); | ||
| clonedWaypoints.unshift(waypoint); | ||
| Onyx.merge(ONYXKEYS.NVP_RECENT_WAYPOINTS, clonedWaypoints.slice(0, 5)); |
There was a problem hiding this comment.
I was thinking of doing slice 0, 4 first and add the other item. but not a blocker
|
@tgolen Should I except to be able to create a distance request? I don't see an option to create the request, but the waypoints are saved just by changing the address without creating the request (bug or feature?) 
|
|
What you're seeing is expected. The option to save the request is coming in
another PR. It's fine that the recent addresses are updated without saving
the request to the server yet. This is so that it will also work offline.
…On Mon, Aug 28, 2023 at 10:49 AM Abdelhafidh Belalia < ***@***.***> wrote:
@tgolen <https://github.com/tgolen> Should I except to be able to create
a distance request? I don't see an option to create the request, but the
waypoints are saved just by changing the address without creating the
request (bug or feature?)
[image: Screenshot 2023-08-28 at 5 49 07 PM]
<https://user-images.githubusercontent.com/16493223/263767954-04d06869-9562-4b9f-b09a-c32ad738b9af.png>
—
Reply to this email directly, view it on GitHub
<#25974 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMAB3PK3D6CYW4VJEHI63XXTD2TANCNFSM6AAAAAA36643UY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Reviewer Checklist
Screenshots/VideosWeb
Mobile Web - Chrome
Mobile Web - Safari
Desktopdesktop.moviOS
Android
|
|
Probably unrelated: Once you go offline the map will be "pending" even after you come back online |
|
@tylerkaraszewski Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
I will go ahead and handle internal review since I am already familiar with the code |
I'm going to handle some of these QOL improvements in a future App PR |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
| @@ -53,7 +54,7 @@ const defaultProps = { | |||
| 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.
wonder why this wasn't caught during PR CI checks
There was a problem hiding this comment.
Yes. It got caught in our PR but not here 🤔 🤔
There was a problem hiding this comment.
this is handled by @allroundexperts now
There was a problem hiding this comment.
Oh wow, yes... I'm surprised I missed it and doubly surprised that the linter missed it! Thanks for handling this.
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.59-0 🚀
|
| @@ -87,6 +93,12 @@ function saveWaypoint(transactionID, index, waypoint) { | |||
| }, | |||
| }, | |||
| }); | |||
| const recentWaypointAlreadyExists = _.find(recentWaypoints, (recentWaypoint) => recentWaypoint.address === waypoint.address); | |||
There was a problem hiding this comment.
FYI this caused a deploy blocker
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.60-0 🚀
|
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.60-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.59-5 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.60-3 🚀
|
Details
This PR shows your most recently chosen waypoints when creating a distance request and selecting addresses.
Fixed Issues
$ #22714
Tests
88 kearnyOffline tests
Same as the above
QA Steps
Same as the tests above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android