-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
fix: Update second Allow location access modal on web #51709
base: main
Are you sure you want to change the base?
Conversation
@alitoshmatov 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] |
@truph01 After not allowing location access the modal is not appearing in second submit. Maybe it is because I am in newer version of chrome where it offer different options for choosing allowing access Screen.Recording.2024-11-03.at.9.37.05.PM.mov |
@alitoshmatov Ah, I see it. Do you think it's actually related to our PR, given that we're focusing on updating the modal (title, subtitle, buttons) rather than modifying the logic for when it should be displayed? |
I guess not, but would be great if we considered this possibility as well. |
@truph01 I am also noticing that in your ios safari recording, the modal is shown second time you submit a scan but clicking continue does not show native location permission modal. In my simulator after I reject location first time, no modal is shown in second scan request permission-safari.mp4 |
In ios also the modal is shown only in first scan request. After rejecting native permission modal first time submitting second invoice request is not triggering modal permission-ios.mp4 |
That is three places – Ios native, Ios safari, Mac chrome which shows only first modal not the second modal with "Got it" button or "Settings" button. If this is accepted behavior we should reconsider testing steps to prevent further confusion |
@alitoshmatov We are encountering different behavior in Chrome, maybe because of the Chrome version (Here is from @alitoshmatov and the one in the screen recording section is from my side). So, I wonder how I can write the test steps properly in Chrome? cc @Julesssss |
I think that sounds like our logic that only prompts the user for location once every 7 days? (each client tracks this seprerately) I agree we need to have well defined test steps. After retesting with the 7 day throttle disabled lets see if our experiences match. To help with this I branched from these changes HERE with that logic removed, and test builds are currently being built...
Also, this screenshot is incorrect for mobile, can you change it to the native permission modal so that QA aren't confused. |
Explanation of Change
Fixed Issues
$ #50601
PROPOSAL: #50601 (comment)
Tests
Test 1: For [Chrome]
Choose "Block".
FAB > Submit expense > Scan > Upload receipt.
In confirmation page, click "Submit expense". Verify the below modal is displayed:
Test 2: For Android/IOS app
Choose "Don't Allow".
FAB > Submit expense > Scan > Upload receipt.
In confirmation page, click "Submit expense". Verify the below modal is displayed:
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
output.mp4
Android: mWeb Chrome
Screen.Recording.2024-10-30.at.08.47.54.mov
iOS: Native
Screen.Recording.2024-10-30.at.08.41.50.mov
iOS: mWeb Safari
Screen.Recording.2024-10-30.at.09.14.04.mov
MacOS: Chrome / Safari
output.mp4
MacOS: Desktop
Screen.Recording.2024-10-30.at.09.11.28.mov