Skip to content
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

[$250] Update second Allow location access modal on web #50601

Open
4 tasks
jamesdeanexpensify opened this issue Oct 10, 2024 · 32 comments
Open
4 tasks

[$250] Update second Allow location access modal on web #50601

jamesdeanexpensify opened this issue Oct 10, 2024 · 32 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Overdue

Comments

@jamesdeanexpensify
Copy link
Contributor

jamesdeanexpensify commented Oct 10, 2024

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
2024-10-10_09-11-41.mp4
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021847227311681373401
  • Upwork Job ID: 1847227311681373401
  • Last Price Increase: 2024-10-18
Issue OwnerCurrent Issue Owner: @alitoshmatov
@jamesdeanexpensify jamesdeanexpensify added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 10, 2024
Copy link

melvin-bot bot commented Oct 10, 2024

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.

@truph01
Copy link
Contributor

truph01 commented Oct 11, 2024

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?

if so, the startPermissionFlow is set to true then this useEffect is called:

useEffect(() => {
if (!startPermissionFlow) {
return;
}
getLocationPermission().then((status) => {
if (status === RESULTS.GRANTED || status === RESULTS.LIMITED) {
return onGrant();
}
setShowModal(true);
setHasError(status === RESULTS.BLOCKED);
});
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- We only want to run this effect when startPermissionFlow changes
}, [startPermissionFlow]);

  • 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:

useEffect(() => {
if (!startPermissionFlow) {
return;
}
getLocationPermission().then((status) => {
if (status === RESULTS.GRANTED || status === RESULTS.LIMITED) {
return onGrant();
}
setShowModal(true);
setHasError(status === RESULTS.BLOCKED);
});
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- We only want to run this effect when startPermissionFlow changes
}, [startPermissionFlow]);

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:

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

const handledBlockedPermission = (cb: () => void) => () => {
if (hasError) {
if (Linking.openSettings) {
Linking.openSettings();
}
setShowModal(false);
setHasError(false);
resetPermissionFlow();
return;
}
cb();
};

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

title={translate(hasError ? 'receipt.locationErrorTitle' : 'receipt.locationAccessTitle')}

and prompt:

prompt={translate(hasError ? 'receipt.locationErrorMessage' : 'receipt.locationAccessMessage')}

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
Copy link
Contributor

truph01 commented Oct 11, 2024

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

@Julesssss Julesssss self-assigned this Oct 11, 2024
@jamesdeanexpensify
Copy link
Contributor Author

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

@twilight2294
Copy link
Contributor

twilight2294 commented Oct 11, 2024

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

@truph01
Copy link
Contributor

truph01 commented Oct 11, 2024

@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.

@melvin-bot melvin-bot bot added the Overdue label Oct 14, 2024
Copy link

melvin-bot bot commented Oct 14, 2024

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

@Christinadobrzyn
Copy link
Contributor

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?

@melvin-bot melvin-bot bot removed the Overdue label Oct 15, 2024
@truph01
Copy link
Contributor

truph01 commented Oct 15, 2024

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

Yes

@Christinadobrzyn
Copy link
Contributor

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

@Julesssss
Copy link
Contributor

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

@jamesdeanexpensify
Copy link
Contributor Author

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

@jamesdeanexpensify
Copy link
Contributor Author

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

@twilight2294
Copy link
Contributor

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

Yes

@twilight2294
Copy link
Contributor

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

@twilight2294
Copy link
Contributor

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?

@jamesdeanexpensify
Copy link
Contributor Author

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

2024-10-10_09-11-41.mp4

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

@truph01
Copy link
Contributor

truph01 commented Oct 16, 2024

Just for alignment purposes - by "native prompt" on web, are we all talking about the second screenshot #50601 (comment)?

  • 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.

@jamesdeanexpensify
Copy link
Contributor Author

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

@jamesdeanexpensify
Copy link
Contributor Author

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
2024-10-10_09-11-41.mp4

@jamesdeanexpensify jamesdeanexpensify changed the title Remove "Allow location access" modal on web Updated second Allow location access modal on web Oct 17, 2024
@jamesdeanexpensify jamesdeanexpensify changed the title Updated second Allow location access modal on web Update second Allow location access modal on web Oct 17, 2024
@twilight2294
Copy link
Contributor

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

@truph01
Copy link
Contributor

truph01 commented Oct 17, 2024

Proposal updated

@jamesdeanexpensify
Copy link
Contributor Author

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.

@twilight2294
Copy link
Contributor

Proposal Updated

Can you explain a bit more what you mean by #50601 (comment)? 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

@jamesdeanexpensify
Copy link
Contributor Author

@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.

@Julesssss
Copy link
Contributor

@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 Julesssss added the External Added to denote the issue can be worked on by a contributor label Oct 18, 2024
@melvin-bot melvin-bot bot changed the title Update second Allow location access modal on web [$250] Update second Allow location access modal on web Oct 18, 2024
Copy link

melvin-bot bot commented Oct 18, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 18, 2024
Copy link

melvin-bot bot commented Oct 18, 2024

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

Copy link

melvin-bot bot commented Oct 21, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Oct 21, 2024
@Christinadobrzyn
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Oct 22, 2024
Copy link

melvin-bot bot commented Oct 24, 2024

@Julesssss @Christinadobrzyn @alitoshmatov this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added the Overdue label Oct 24, 2024
@Julesssss
Copy link
Contributor

Hi @alitoshmatov, are you able to review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Overdue
Projects
Status: Polish
Development

No branches or pull requests

6 participants