App icon indicating copy to clipboard operation
App copied to clipboard

[HOLD for payment 2024-11-29] [$250] Update second `Allow location access` modal on web

Open jamesdeanexpensify opened this issue 1 year ago • 61 comments

On web only, for the second Allow location access modal only (the one with the Got it button):

  • [ ] Fix the bug where you can't submit after clicking Got it (see video below)
  • [ ] Fix the bug where, after clicking the Got it button, the same button quickly flickers to say Continue (same video below)
  • [ ] Update the copy on the modal slightly to:

Allow location access Location access helps us keep your timezone and currency accurate wherever you go. Please allow location access from your device's permission settings.

  • [ ] Remove the Not now button entirely because it serves no purpose in this case

https://github.com/user-attachments/assets/c9dd15b2-ab3b-46c9-ae86-7d5c43d00bd5

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021847227311681373401
  • Upwork Job ID: 1847227311681373401
  • Last Price Increase: 2024-10-25
  • Automatic offers:
    • alitoshmatov | Reviewer | 104624114
    • truph01 | Contributor | 104624115
Issue OwnerCurrent Issue Owner: @alitoshmatov

jamesdeanexpensify avatar Oct 10 '24 21:10 jamesdeanexpensify

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Oct 10 '24 21:10 melvin-bot[bot]

Edited by proposal-police: This proposal was edited at 2024-10-17 18:30:45 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

  • Remove "Allow location access" modal on web

What is the root cause of that problem?

  • When pressing on submit button, we check whether we should start a permission flow: https://github.com/Expensify/App/blob/7bec576b2e77f83c627aad42b54a8adf99e85214/src/pages/iou/request/step/IOURequestStepConfirmation.tsx#L592

if so, the startPermissionFlow is set to true then this useEffect is called: https://github.com/Expensify/App/blob/4c90d62a6a56ff0000031eceeb9195d30a041a8a/src/components/LocationPermissionModal/index.tsx#L22-L36

  • First, it checks the current permission. If it is RESULTS.GRANTED or RESULTS.LIMITED, it don't show any modal. Otherwise, the modal is shown.

  • This modal is to:

pre-prompt the user for their location access instead of using the native prompt from Apple/Google that would penalize you if you asked for location access too many times. This way, we can prompt users with our own modal on mobile to get a response before the native prompt, avoiding any penalties if the answer is "no" but still allowing us to ask as many times as we want.

  • When user chooses "Confirm" button, then the native prompt is shown. In this issue, we are going to remove pre-prompt.

What changes do you think we should make in order to solve the problem?

  • As I mentioned above, In this issue, we are going to remove pre-prompt. So the permission flow will be: Checking existing permission > displaying native prompt > sending expense instead of Checking existing permission > display custom pre-prompt > displaying native prompt > sending expense.

  • To match the new flow, we can create a new index.website.ts, its logic is the same as index.ts, just need to update the useEffect:

https://github.com/Expensify/App/blob/4c90d62a6a56ff0000031eceeb9195d30a041a8a/src/components/LocationPermissionModal/index.tsx#L22-L36

useEffect(() => {
    if (!startPermissionFlow) {
        return;
    }

    getLocationPermission().then((status) => {
        if (status === RESULTS.GRANTED || status === RESULTS.LIMITED) {
            return onGrant();
        }
        
        // If the user has previously denied the permission, we still need to show a modal
        // to inform them that they must go to the settings page to enable location permission.
        if (status === RESULTS.BLOCKED) {
            setShowModal(true);
            setHasError(status === RESULTS.BLOCKED);
        } else {
            // If the native permission prompt hasn't been shown before, show it now.
            grantLocationPermission();
        }
    });
    
    // eslint-disable-next-line react-hooks/exhaustive-deps -- We only want this effect to run when startPermissionFlow changes.
}, [startPermissionFlow]);

What alternative solutions did you explore? (Optional)

Solution for new design:

  1. Update:

https://github.com/Expensify/App/blob/4c90d62a6a56ff0000031eceeb9195d30a041a8a/src/components/LocationPermissionModal/index.tsx#L85

        <ConfirmModal
            shouldShowCancelButton={!(isWeb && hasError)}
            onModalHide={() => {
                setHasError(false);
                resetPermissionFlow();
            }}
  1. Update:

https://github.com/Expensify/App/blob/4c90d62a6a56ff0000031eceeb9195d30a041a8a/src/components/LocationPermissionModal/index.tsx#L38-L49

    const handledBlockedPermission = (cb: () => void) => () => {
        if (hasError) {
            if (Linking.openSettings) {
                Linking.openSettings();
            } else {
                onDeny?.();
            }
            setShowModal(false);
            return;
        }
        cb();
    };
  1. Update the title:

https://github.com/Expensify/App/blob/4c90d62a6a56ff0000031eceeb9195d30a041a8a/src/components/LocationPermissionModal/index.tsx#L94

and prompt:

https://github.com/Expensify/App/blob/4c90d62a6a56ff0000031eceeb9195d30a041a8a/src/components/LocationPermissionModal/index.tsx#L92

to match the new design:

Allow location access Location access helps us keep your timezone and currency accurate wherever you go. Please allow location access from your device's permission settings.

It is very straightforward:

            prompt={translate(hasError ? (isWeb ? {new_prompt} : 'receipt.locationErrorMessage') : 'receipt.locationAccessMessage')}
            title={translate(hasError ? (isWeb ? {new_title} : 'receipt.locationErrorTitle') : 'receipt.locationAccessTitle')}

truph01 avatar Oct 11 '24 01:10 truph01

@jamesdeanexpensify We don't want to display the pre-prompt but still display the native prompt on web, no?

truph01 avatar Oct 11 '24 01:10 truph01

Yes, we would still show the native prompt on web. Thanks!

jamesdeanexpensify avatar Oct 11 '24 17:10 jamesdeanexpensify

Edited by proposal-police: This proposal was edited at 2024-10-15 11:18:52 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Remove "Allow location access" modal on web

What is the root cause of that problem?

Feature request

What changes do you think we should make in order to solve the problem?

New Solution according to expected results:

  1. Call skipLocationPermission when we click got it on web:
  2. Introduce shouldShowCancelButton prop to ConfirmModal:

And update the locationErrorMessage message to Location access helps us keep your timezone and currency accurate wherever you go. Please allow location access from your device's permission settings., if this doesn't fit screen then minor style changes would also be needed. But

        <ConfirmModal
            onConfirm={(hasError && isWeb) ? skipLocationPermission : grantLocationPermission}
            shouldShowCancelButton={!(hasError && isWeb)}

These changes are enough to match the expected results in this case

twilight2294 avatar Oct 11 '24 19:10 twilight2294

@jamesdeanexpensify @Julesssss In my opinion, I don’t think we can fully hide the modal

Previously, when the user clicked the submit button and the native prompt hadn’t been shown yet, we displayed the pre-prompt modal:

image

If the user chose 'Continue,' but location permission wasn’t granted, the native prompt wouldn’t appear in the future. That's why, the next time, we display a modal to inform them that they need to manually enable location permission:

image

So, the 2nd modal should not be hidden.

truph01 avatar Oct 11 '24 23:10 truph01

@Julesssss, @Christinadobrzyn Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Oct 14 '24 18:10 melvin-bot[bot]

Thanks for the feedback @truph01!

Sorry, just catching up here, can you confirm the difference you are proposing? I think it's that we only show the "Not Now"/"Got it" prompt on the web and not the pre-prompt? Is that right?

Christinadobrzyn avatar Oct 15 '24 03:10 Christinadobrzyn

I think it's that we only show the "Not Now"/"Got it" prompt on the web and not the pre-prompt

Yes

truph01 avatar Oct 15 '24 03:10 truph01

I think this makes sense - do you @Julesssss @jamesdeanexpensify? Do you think we need a consensus from the team about this?

Christinadobrzyn avatar Oct 16 '24 06:10 Christinadobrzyn

That seems to be what we agreed, but lets check in with James first.

Julesssss avatar Oct 16 '24 12:10 Julesssss

We're discussing this one internally and may recommend an alternative course of action. Stay tuned!

jamesdeanexpensify avatar Oct 16 '24 19:10 jamesdeanexpensify

Just for alignment purposes - by "native prompt" on web, are we all talking about the second screenshot here?

jamesdeanexpensify avatar Oct 16 '24 20:10 jamesdeanexpensify

Just for alignment purposes - by "native prompt" on web, are we all talking about the second screenshot here?

Yes

twilight2294 avatar Oct 16 '24 20:10 twilight2294

The video you attached is not being played, can you add another

twilight2294 avatar Oct 16 '24 20:10 twilight2294

I actually think on web we can probably leave these prompts entirely in place, but (again, on web only):

@jamesdeanexpensify this means that we only show the prompt on native devices right?

twilight2294 avatar Oct 16 '24 20:10 twilight2294

Hmmm...that video is working for me. Here it is again:

https://github.com/user-attachments/assets/8de2df93-6455-4663-aec6-ed29212f6e17

And sorry, what do you mean by "only show the prompt on native devices"?

jamesdeanexpensify avatar Oct 16 '24 20:10 jamesdeanexpensify

Just for alignment purposes - by "native prompt" on web, are we all talking about the second screenshot https://github.com/Expensify/App/issues/50601#issuecomment-2408219295?

  • When users sign-in on a new device and are not asked about the location permission before. If they create a scan expense, the modal below is displayed, it is "pre-prompt":

Screenshot 2024-10-17 at 06 34 00

  • If the user chooses "Continue", the modal below is displayed (in top left screen), it is "native prompt":

Screenshot 2024-10-17 at 06 31 15

  • If the user chooses "Block", and then creates an other expense, the modal below is displayed:

Screenshot 2024-10-17 at 06 34 00

  • If the user chooses "Got it", the modal will close.

truph01 avatar Oct 16 '24 23:10 truph01

Ah, that's super helpful! Bringing this back to the team to chat about next steps again. Thanks!

jamesdeanexpensify avatar Oct 17 '24 00:10 jamesdeanexpensify

OK, here is where we landed:

On web only, for the second Allow location access modal only (the one with the Got it button):

  • [ ] Fix the bug where you can't submit after clicking Got it (see video below)
  • [ ] Fix the bug where, after clicking the Got it button, the same button quickly flickers to say Continue (same video below)
  • [ ] Update the copy on the modal slightly to:

Allow location access Location access helps us keep your timezone and currency accurate wherever you go. Please allow location access from your device's permission settings.

  • [ ] Remove the Not now button entirely because it serves no purpose in this case

https://github.com/user-attachments/assets/db337e01-db1d-40e3-b7e9-3c74b2776ede

jamesdeanexpensify avatar Oct 17 '24 17:10 jamesdeanexpensify

@jamesdeanexpensify so by only web we mean that there should be no change to the behaviour on native platforms right ?

twilight2294 avatar Oct 17 '24 18:10 twilight2294

Proposal updated

truph01 avatar Oct 17 '24 18:10 truph01

Can you explain a bit more what you mean by that? Just want to make sure I'm understanding, thanks! I'm not a BZ person so it's helpful.

jamesdeanexpensify avatar Oct 17 '24 18:10 jamesdeanexpensify

Proposal Updated

Can you explain a bit more what you mean by https://github.com/Expensify/App/issues/50601#issuecomment-2420238123? Just want to make sure I'm understanding, thanks! I'm not a BZ person so it's helpful.

Thanks for asking, I got the context, i updated the proposal accordingly

twilight2294 avatar Oct 17 '24 19:10 twilight2294

@Christinadobrzyn @Julesssss it looks like we have two proposals (here and here) to review, and I've updated the tasks in the OP up top! Quick note - we're not removing any modals now, only adjusting an existing one.

jamesdeanexpensify avatar Oct 17 '24 22:10 jamesdeanexpensify

@jamesdeanexpensify so by only web we mean that there should be no change to the behaviour on native platforms right ?

Yeah, just web will change.

Julesssss avatar Oct 18 '24 10:10 Julesssss

Job added to Upwork: https://www.upwork.com/jobs/~021847227311681373401

melvin-bot[bot] avatar Oct 18 '24 10:10 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @alitoshmatov (External)

melvin-bot[bot] avatar Oct 18 '24 10:10 melvin-bot[bot]

@Julesssss, @Christinadobrzyn, @alitoshmatov Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Oct 21 '24 18:10 melvin-bot[bot]

@alitoshmatov can you please check out the proposals when you have a moment? TY!

Christinadobrzyn avatar Oct 22 '24 02:10 Christinadobrzyn