Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@rafecolton @mkhutornyi Please review the implementation details in OP. Also, please note the following:
|
rafecolton
left a comment
rafecolton
left a comment
There was a problem hiding this comment.
Everywhere we use long_name or short_name in both code and comments in src/components/AddressSearch/index.tsx and src/libs/GooglePlacesUtils.ts, it needs to be changed to longName and shortName respectively. With that small change, this works great!
| name: details.name, | ||
| lat: details.location?.latitude ?? details.geometry.location?.lat ?? 0, | ||
| lng: details.location?.longitude ?? details.geometry.location?.lng ?? 0, | ||
| name: autocompleteData?.structured_formatting?.main_text ?? details?.name ?? '', |
There was a problem hiding this comment.
details.name isn't the same as it was in the old API, the format here is places/<id> so not really good to use here. The next closest thing is details?. shortFormattedAddress
There was a problem hiding this comment.
In this context, the details.name is not used from old API. Instead, it indicates a predefined option i.e. saved places in our app. So, I think we need to keep
details.name
There was a problem hiding this comment.
I don't understand what you mean, can you show me? The way I understand this code, the value in autocompleteData.structured_formatting.main_text in the new API is most similar to details.name from the old api. However, details.name is no longer a good fallback, as in the new API, it does not contain a short version of the address, but something else entirely
There was a problem hiding this comment.
I don't understand what you mean, can you show me?
Please find the test video that demonstrates the use of predefined places. This is the reason why I think we need to keep details.name.
47085-predefined-places.mp4
There was a problem hiding this comment.
Hm ok. Can you take a look at how this is utilized when changing your address on the profile settings page as well? I wonder if we need different values in the two places.
| const values = { | ||
| street: `${streetNumber} ${streetName}`.trim(), | ||
| name: details.name ?? '', | ||
| name: autocompleteData?.structured_formatting?.main_text ?? details?.name ?? '', |
|
Sorry for the delay here. I will look into this today and revert. |
@rafecolton I am not able to test as I still get 47085-api-issue.mp4 |
|
Ah, looks like you tested a couple hours before the code was on staging. Sorry for the confusion, I was asking in general. It's on staging now, so please try again and LMK. |
Yeah @rafecolton. The BE changes are reflected in staging as I do not get While I can fetch the Can you please help confirm in BE if the 47085-placedetails-issue.mp4 |
|
That makes sense. See my prior review. |
@rafecolton Sorry if I am missing something in your review here but can you please point out which one? |
|
If you're seeing the API return data and the UI is not working, that means we're parsing it wrong in the front end. You can print it with That's what I did when I posted #47085 (review), which explains what to do to fix the error you're seeing. Edit: I misread your response, I see now you said you're not getting back any data at all from the API. Can you please start by making the change I requested in my review? I tested it locally and it works with that change, so I know that change is required. It looks like the request format may have changed from when I tested based on your updates to the library function so once you make the changes I requested here, I'll test again against the BE proxy locally to see what's up. |
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
it needs to be changed to
longNameandshortNamerespectively
Instead of longName and shortName the response now consists of longText and shortText. Looks like the response format has changed as can be seen here. I got this response when requesting via API explorer on Google Maps platform. With this data, the FE works well.
To test this, please use Statue of Liberty as search text and select Statue of Liberty National Monument to request the place details api. For testing purposes, I am constructing a response when BE sends back empty response as referenced here and it works well. Here is a test video for this.
47085-api-issue2.mp4
| name: details.name, | ||
| lat: details.location?.latitude ?? details.geometry.location?.lat ?? 0, | ||
| lng: details.location?.longitude ?? details.geometry.location?.lng ?? 0, | ||
| name: autocompleteData?.structured_formatting?.main_text ?? details?.name ?? '', |
There was a problem hiding this comment.
In this context, the details.name is not used from old API. Instead, it indicates a predefined option i.e. saved places in our app. So, I think we need to keep
details.name
| const values = { | ||
| street: `${streetNumber} ${streetName}`.trim(), | ||
| name: details.name ?? '', | ||
| name: autocompleteData?.structured_formatting?.main_text ?? details?.name ?? '', |
@rafecolton I have addressed the review comments. Hopefully we will unravel the mystery of empty response when we test the BE proxy locally. I have added the I have added |
|
@rafecolton While we address the issue of This issue related to As our current focus is to address the 47085-api-issue3.mp4 |
@rafecolton I just tested 47085-place-details-works-1.mp4 |
|
@rafecolton So the only pending issue is the comment here concerning |
Yes you are correct, it's diff --git a/src/components/AddressSearch/index.tsx b/src/components/AddressSearch/index.tsx
index 712c364b646..f18c6c63272 100644
--- a/src/components/AddressSearch/index.tsx
+++ b/src/components/AddressSearch/index.tsx
@@ -135,27 +135,27 @@ function AddressSearch(
country: countryPrimary,
} = GooglePlacesUtils.getAddressComponents(addressComponents, {
// eslint-disable-next-line @typescript-eslint/naming-convention
- street_number: 'long_name',
- route: 'long_name',
- subpremise: 'long_name',
- locality: 'long_name',
- sublocality: 'long_name',
+ street_number: 'longText',
+ route: 'longText',
+ subpremise: 'longText',
+ locality: 'longText',
+ sublocality: 'longText',
// eslint-disable-next-line @typescript-eslint/naming-convention
- postal_town: 'long_name',
+ postal_town: 'longText',
// eslint-disable-next-line @typescript-eslint/naming-convention
- postal_code: 'long_name',
+ postal_code: 'longText',
// eslint-disable-next-line @typescript-eslint/naming-convention
- administrative_area_level_1: 'short_name',
+ administrative_area_level_1: 'shortText',
// eslint-disable-next-line @typescript-eslint/naming-convention
- administrative_area_level_2: 'long_name',
- country: 'short_name',
+ administrative_area_level_2: 'longText',
+ country: 'shortText',
});
- // The state's iso code (short_name) is needed for the StatePicker component but we also
- // need the state's full name (long_name) when we render the state in a TextInput.
+ // The state's iso code (shortText) is needed for the StatePicker component but we also
+ // need the state's full name (longText) when we render the state in a TextInput.
const {administrative_area_level_1: longStateName} = GooglePlacesUtils.getAddressComponents(addressComponents, {
// eslint-disable-next-line @typescript-eslint/naming-convention
- administrative_area_level_1: 'long_name',
+ administrative_area_level_1: 'longText',
});
// Make sure that the order of keys remains such that the country is always set above the state.
diff --git a/src/libs/GooglePlacesUtils.ts b/src/libs/GooglePlacesUtils.ts
index 312a0dc61c1..147d6933435 100644
--- a/src/libs/GooglePlacesUtils.ts
+++ b/src/libs/GooglePlacesUtils.ts
@@ -1,8 +1,8 @@
type AddressComponent = {
// eslint-disable-next-line @typescript-eslint/naming-convention
- long_name: string;
+ longText: string;
// eslint-disable-next-line @typescript-eslint/naming-convention
- short_name: string;
+ shortText: string;
types: string[];
};
type FieldsToExtract = Record<string, Exclude<keyof AddressComponent, 'types'>>;
@@ -11,8 +11,8 @@ type FieldsToExtract = Record<string, Exclude<keyof AddressComponent, 'types'>>;
* Finds an address component by type, and returns the value associated to key. Each address component object
* inside the addressComponents array has the following structure:
* [{
- * long_name: "New York",
- * short_name: "New York",
+ * longText: "New York",
+ * shortText: "New York",
* types: [ "locality", "political" ]
* }]
*/ |
|
Looking at the waypoint issue now |
|
Was able to reproduce the location bias issue. You are right, it looks like the problem is Google isn't handling URI-encoded data that's multiple layers deep, even though it's encoded correctly. Experimenting a little to see if it's better to send as JSON from the front-end or decode in PHP. Probably the former so this works for people not using a proxy |
There was a problem hiding this comment.
PR for back-end changes is up for review. I tested and confirmed it works with the diff from my review here and the diff posted in this comment. It fixes the locationBias issue you described. Please make both of those changes, and I will let you know when it's ready to test on staging.
Please also reply to my comment here 🙏
This is almost ready! Thank you for your persistence and diligence in checking edge cases.
|
Library was published, so placing this in |
melvin-bot
bot
requested review from
mkhutornyi
and removed request for
a team
|
@mkhutornyi 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] |
rafecolton
left a comment
rafecolton
left a comment
There was a problem hiding this comment.
Tested this out locally and it worked great! @mkhutornyi @rojiphil please complete your cross-platform testing and checklists, and assuming no unexpected errors, I will merge.
Great work!
@rafecolton On native iOS session token generation is failing due to unsupported crypto.getRandomValues. Please hold merge until I finish investigation on this.
|
|
That also happens on android. Looks like this is the only way to support on native: 
|
|
Is this happening on |
This issue is specific to this PR only. The mains will not be impacted as it still points to 2.5.6 version of library. It is only after we merge this PR that the bumped up 2.5.7 version will kick-in. |
I have opened a follow-up PR here to address this issue. More specific comment here on the fix. I think we can keep this simple by using a random generator for generating the uuid. I have tested and it works for iOS and android native devices. |
|
@rafecolton We have another issue with Android Native platform. This issue and the fix are mentioned here. But on adding this, the
47085-desktop-cors-issue.mp4 |
|
I did notice the CORS error when testing on dev, so I didn't add the header. It's interesting that it's required for android. I dug into it a bit and it was quite the rabbit hole, but since it seems to be required here, I'll check it out. |
|
Linked issue has been closed. |
3 participants
@rafecolton @mkhutornyi
Details
This PR implements the scope limited to the places migration as used in NewDot.
Dependencies:
react-native-google-places-autocompletelibrary which is implemented in this PRPlaces API (New)in Google CloudAutocomplete (New)andPlace Details(New)The implementation details for the relevant steps as mentioned here are as follows:
Let us introduce a
isNewPlacesAPIproperty inGooglePlacesAutocompletecomponent here to introduce support forPlaces (New)api. By default, we can set this tofalsehere so that the current implementation ofPlacesapi is used by default. This way, we do not break existing systems using the oldPlacesAPI. Further, to make use of thePlaces API (New), the FE can sendisNewPlacesAPIhere.For
Autocomplete (New)request, we can send a HTTP POST request here and also set the new url for POST i.e.${url}/v1/places:autocomplete. Further, we can send the requestbodywith the supported parameters here.In addition, we also need to update the API for
Place Details (New)with url${url}/v1/places/${rowData.place_id}hereFormat changes include changing the request parameters to
languageCode,includedRegionCodesandlocationBiashere. For the response though, we need to filter the results and handle the format changes. This is done here and hereLet us introduce a
fieldsproperty inGooglePlacesAutocompletecomponent here to specify the desired response fields. By default, we can set this to*here so that all the fields are sent back. In our case, we setfieldsto make use of Location Only (Place Details) SKU thereby reducing our API cost. The specific value offieldsis set here.To use sessions, we can create a
sessionTokenstate here and useuuidlibrary to generate v4 uuids for the session. This session token will be used in everyAutocomplete (New)API here. And the session token will be reset to a new uuid here after using it for the last time in thePlace Details (New)API hereSince the
responsehas changedPlace Details (New)API, there will not be anystatus. Hence, we can depend onidhere and pass thedetailsto theonPresshandler. The FE saveLocationDetails is modified to support the format changes in thePlace Details (New)API response.Fixed Issues
$ #46771
PROPOSAL:
Test
Steps:
Distancetap and clickStartto setup a waypointAddressinput fieldstartwaypointStopto setup a waypointstopwaypointOffline tests
Same as the steps in Tests Section.
QA Steps
Same as the steps in Tests Section.
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 */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
MacOS: Safari
TODO
iOS: mWeb Safari
TODO
MacOS: Desktop
TODO
iOS: Native
TODO
Android: Native
TODO
Android: mWeb Chrome
TODO