[Payment due][$1000] Room - Side panel does not close when clicking outside of panel

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.24-4
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Click on FAB > Start chat > room > workspace
2. Click outside of the side panel

Expected Result:

Side panel should close as it does when clicking on outside of the panel anywhere in the app

Actual Result:

Side panel stays open but navigates one step back

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6339423_1705008289448.clickingOnOutsideBoxRoom.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011ab450f9ef4bcadd
• Upwork Job ID: 1745558199758635008
• Last Price Increase: 2024-02-21
• Automatic offers:
• hoangzinh | Reviewer | 28104675
• neonbhai | Contributor | 28104676
• Krishna2323 | Contributor | 28142924

Issue Owner

Current Issue Owner: @kadiealexander

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

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

[Krishna2323]

Proposal

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

Room - Side panel does not close when clicking outside of panel

What is the root cause of that problem?

We are only hiding picker modal when onClose is called inside ValuePicker.

App/src/components/ValuePicker/index.js

Lines 59 to 61 in 63ed47d

	const hidePickerModal = () => {
	setIsPickerVisible(false);
	};

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

Option 1:
We need to modify ValueSelectorModal to accept a callback for onBackdropPress event also and then we will pass a function that calls Navigation.dismissModal() when onBackdropPress event is fired inside ValueSelectorModal. We also need to introduce onBackdropPress callback prop inside BaseModal.

Option 2:
Or we can use onBackButtonPress on HeaderWithBackButton to hide the valuePicker and use onClose to dismiss modal using Navigation.dismissModal() or Navigation.goBack(), inside ValueSelectorModal we can separate logic.

Option 3:
We will pass a callback to BaseModal directly from ValueSelectorModal that will run on backdropPress and will call Navigation.dismissModal() or Navigation.goBack(). Still we need to add a onBackdropPress prop inside BaseModal. We will be passing the callback to BaseModal as a default value. Like we did it here:

App/src/libs/Navigation/AppNavigator/Navigators/RightModalNavigator.tsx

Lines 28 to 36 in b6a18b1

	<Overlay
	onPress={() => {
	if (isExecutingRef.current) {
	return;
	}
	isExecutingRef.current = true;
	navigation.goBack();
	}}
	/>

Result

workspace_modal_hide.mp4

Alternative:

We will make this a page on the RHP (Exactly like #26742). We will:

• Define a route in ROUTE.ts
• Add it to ModalStackNavigators.tsx and to linkingConfig.js
• And move the content that's inside the modal into the new page that can be pushed on the RHP stack.
• We will use URL encoding to share the data

[yuetong3yu]

Feel like it's expected instead a bug.

[melvin-bot]

📣 @yuetong3yu! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[yuetong3yu]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~019f5d35fda67374fb

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[neonbhai]

Proposal

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

Room - Side panel does not close when clicking outside of panel

What is the root cause of that problem?

The workspace picker is rendered as a modal here, so on clicking outside, it dismisses. We have it in RHP but not on a route causing unexpected behaviour.



It looks like a Right Hand Panel, but acts like a modal, breaking user expectations.

We should add it to a route exactly how we refactored Currency Selection modal, Year Picker (pr up) and Country Picker modal (all these had the same issue).

What changes do you think we should make in order

We will make this a page on the RHP (Exactly like #26742). We will:

• Define a route in ROUTE.ts
• Add it to ModalStackNavigators.tsx and to linkingConfig.js
• And move the content that's inside the modal into the new page that can be pushed on the RHP stack.
• We will use URL encoding to share the data

[hoangzinh]

@Krishna2323 @neonbhai Thanks for your proposals. @neonbhai's proposal looks good to me. It aligns with our discussion in this issue #23725 (comment)

Link to proposal #34394 (comment)

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @danieldoglas, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[Krishna2323]

@hoangzinh, could you please review the details here? It mentions that ValuePicker is intended to function as a modal. I also believe that creating a route and component for each option is a bit excessive. My solution is straightforward and can generically solve the problem with just a few lines of code changes.

[Krishna2323]

@hoangzinh friendly bump to have a look at this #34394 (comment).

[kadiealexander]

@danieldoglas bump!

[hoangzinh]

@kadiealexander please let me review @Krishna2323's comment here #34394 (comment) before we go with selected proposal.

[hoangzinh]

@danieldoglas what do you think about @Krishna2323's proposal?

I just looked at the code and found that the ValuePicker is not only used to pick workspaces, but it's also used to select writeCapability and visibility. It's not easy & hard to use this ValuePicker in other places when we go with @neonbhai's proposal

[melvin-bot]

@danieldoglas, @hoangzinh, @kadiealexander Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[danieldoglas]

Thanks for the discussion here.

Though I agree @Krishna2323 proposals would also solve the issue, I think that @neonbhai's proposal follows the practices we've implemented in other places already.

[danieldoglas]

Reached production last week, soon it will be ready for payment.

[hoangzinh]

@danieldoglas ah we still have a follow-up PR to improve current behavior here #38711. Could you help to change label from "Awaiting Payment" -> "Reviewing"? Thanks

[melvin-bot]

This issue has not been updated in over 15 days. @danieldoglas, @hoangzinh, @kadiealexander, @Krishna2323 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[hoangzinh]

We're still discussing on the follow-up PR #38711

[hoangzinh]

@kadiealexander @dangrous I think we can process payment here and close this issue because the regression issue is just closed #38665 (comment)

[Krishna2323]

@kadiealexander, friendly bump for payments.

[kadiealexander]

Since there was a regression, reducing the payment by 50% as per the contributing guidelines.

Payouts due:

• Contributor: $500 @Krishna2323 (offer)
• Contributor+: $500 @hoangzinh (offer)

Upwork job is here.

[Krishna2323]

@danieldoglas @kadiealexander @hoangzinh, IMO there should be no reduction in payment since the regression was minor and we ended up doing nothing there. I have put a lot of effort into this issue, first in this PR, then in the second PR, and then I have invested a lot of time into the PR to fix the regression, which had no straightforward solution because we didn't want to create separate screens. See #38665 (comment). Pls correct me if I'm wrong here 🙃

[kadiealexander]

@danieldoglas I'll let you decide if the regression penalty should apply here, as I don't have the technical understanding to know if the regression was true.

[danieldoglas]

I agree we can't skip the reduction because of the effort applied.

[kadiealexander]

Thanks Daniel!