-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Allow user to get current location #25990
Allow user to get current location #25990
Conversation
@narefyev91 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] |
@hayata-suenaga @narefyev91 PR is up. I have some UX questions.
To simplify 2 and 3, should we show a form error when the address input has "Your Location" text set manually by the user? Other considerations
I will add all test videos soon after these questions are addressed. Just for a reference, this is how it looks like in chrome Web ChromeWeb.Chrome.mp4 |
@huzaifa-99 Is this only for desktops? |
@huzaifa-99 I couldn't quite understand these questions (I'm sorry 😓 ), could you provide videos showing the flow you're describing 🙇 🙇 🙇 |
Yes the API key is only needed for desktop @hayata-suenaga. |
Apologies, it was for UX and for both 2 and 3 I meant it would look something like the video below. Notice the back-and-forth navigation, and the form error when we try to save the "Your Location" text manually instead of clicking the 'Use Current Location' button. VideoScreen.Recording.2023-08-27.at.2.40.28.AM.movJust wanted to confirm if we should show a form error when the address input has "Your Location" text set manually by the user (again apologies for making it wordy 😅) cc: @hayata-suenaga |
thank you @huzaifa-99 for the PR so far. I gonna address your questions on the UX shortly. I'm working on figuring out the Google API key stuff right now It's weird that Electron calls the Google geolocation API to get the current location. Do you know why it cannot get the location from the OS directly? |
Reviewer Checklist
Screenshots/VideosWeb8mb.video-ThL-w0cMhEHD.mp48mb.video-GOv-6pYhU7kg.mp4Mobile Web - Chrome8mb.video-bdE-PCYvj3WV.mp4Mobile Web - Safariios-web1.mov8mb.video-oIH-0qUvVOse.mp4Desktop8mb.video-Qzm-1zM0vjPu.mp48mb.video-Djc-8ZLhBau3.mp4iOS8mb.video-r0v-sV44FVVD.mp4Android8mb.video-OEH-7pFpveOm.mp4 |
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.
LGTM!
I mostly worried about crash - but if it should not be a part of this PR - and all other items are not so important - than we can move forward - in any case get location is working correctly on all platforms
🎀 👀 🎀 C+ reviewed
Looking good. I'll ask another internal engineer's review just in case 👍 |
Thank you for the feedback @hayata-suenaga.
Apologies i didn't knew that, i created a PR for that Expensify/react-native-x-maps#35. I think i oversteped and maybe we can close that PR.
There is one last concern I have. What if someone lives outside of US and uses their current location? Currently we can only add US locations from the search dropdown. So if a user is from outside US and uses their current location and a US location for waypoints, the distance would be big. In certain cases, a user can get "Route exceeds maximum distance limitation" error. |
This issue is being fixed now and now you can choose address outside US, but I love your attention to details ❤️ |
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.
Looking good! Just a couple of questions
@huzaifa-99 everything is looking good, but I need you to fill out the checklist, and add videos for all platforms please! |
NAB: I remember there was a discussion somewhere why we cannot use ah found this: #25990 (comment) @huzaifa-99 could you give me more detailed summary of the reason why we couldn't use the HOC after the PR is merged (let's prioritize the merge of the PR first) |
yes, I am adding those right now @stitesExpensify
sure. I will add a comment soon after adding videos. |
Checklist completed and videos added. |
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.61-1 🚀
|
I am looking into a deploy blocker linked to this but I am just here to note that Spanish translations were not approved by anyone and they are not right 😢 |
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.61-3 🚀
|
<View> | ||
<Tooltip text={translate('common.close')}> | ||
<PressableWithoutFeedback | ||
onPress={dismissError} |
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.
Coming from #29433.
If the address input is focused, pressing the mouse on this button will trigger blur
and hide the autocomplete component, causing this button to move up (layout shift), thus causing the click event to fail to be triggered on this button.
We add onMouseDown={e => e.preventDefault()}
to prevent blur
event.
Details
Fixed Issues
$ #25855
$ #22707
PROPOSAL: #25855 (comment)
Tests
Prerequisite: Distance request beta must be enabled
Offline tests
Only step 6 from the "Tests" section above doesn't apply in offline tests. All other steps are same as "Tests" section.
QA Steps
Same as "Tests" section above.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Chrome:
Web.Chrome.mp4
Safari:
Web.Safari.mp4
Mobile Web - Chrome
mWeb.Chrome.mp4
Mobile Web - Safari
mWeb.Safari.mp4
Desktop
Desktop.mp4
iOS
IOS.Native.mp4
Android
Android.Native.mp4