-
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
[$1000] Web-Workspace-App does nothing on clicking on 3 dots when modal is open (modal closes for phone) #24284
Comments
Triggered auto assignment to @slafortune ( |
Bug0 Triage Checklist (Main S/O)
|
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open
|
Triggered auto assignment to @PauloGasparSv ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Web-Workspace-App does nothing on clicking on 3 dots when modal is open What is the root cause of that problem?
These are all non-toggle popovers and closing of the popovers not handeled :
What changes do you think we should make in order to solve the problem?
onPress={() => {
if (this.state.isPopupMenuVisible) {
this.hidePopoverMenu();
return;
}
this.showPopoverMenu();
if (this.props.onIconPress) {
this.props.onIconPress();
}
}} note: i am not sure if we should call onIconPress both on opening and closing. currently ThreeDotMenu is used in HeaderView (
const paymentMethodPressed = (nativeEvent, accountType, account, isDefault, methodID) => {
if(shouldShowAddPaymentMenu) {
setShouldShowAddPaymentMenu(false);
return;
}
// rest of code In BaseKYCWall add following changes: (wasn't able to test this properly i dont have beta access) continue(event, iouPaymentType) {
if (this.state.shouldShowAddPaymentMenu) {
this.setState({shouldShowAddPaymentMenu: false});
return;
}
//.....
onPress={() => setIsMenuVisible(!isMenuVisible)}
// or onPress={() => setIsMenuVisible((prev) => !prev)} |
ProposalPlease re-state the problem that we are trying to solve in this issue.App does nothing on clicking 3 dots when modal is open What is the root cause of that problem?When click 3 dots, showPopoverMenu is called instead of toggle App/src/components/ThreeDotsMenu/index.js Lines 89 to 95 in 32326f6
What changes do you think we should make in order to solve the problem?We should update logic similar to VideoChatButtonAndMenu so when click, popover open/closed status should be toggled App/src/components/VideoChatButtonAndMenu/BaseVideoChatButtonAndMenu.js Lines 90 to 100 in 32326f6
|
I don't think we need to block the deploy over this. Removing the label. |
I don't think this is blocker, even not a bug. Just inconsistency issue. |
Toggle popovers:
Non-toggle popovers:
|
are these intentional non toggle popovers or do we need to make them toggle popovers ? |
I tend to close this as not bug. |
Hi @aimane-chnaif, one of the effects of it is that we can write and send anything in background even when menu is open, it doesn't do that for any other modal, I think that should be worth fixing. 2023-08-09.00-00-08.mp4 |
|
📣 @msapple0338! 📣
|
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
@PauloGasparSv and @aimane-chnaif - do you think we should move forward with this or close |
Thks so much for all the help here @aimane-chnaif
I'm not sure. IMO we should go for consistency (but I also don't consider this a "Bug"). Would love to hear your opinion on this too @slafortune If you agree then let's add the |
📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
Hi. Glad meet you here to solve this problem I need to use a onPress() function in react native. |
Current assignee @PauloGasparSv is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
and also we have to use the TouchableWithoutFeedBack component. |
📣 @ruiyun43! 📣
|
I got it. but I think that explain about the solution to solve this problem is not enough with speaking. I want to explain about the solution on call. |
Hi @ruiyun43, welcome to the community, this issue is assigned to me already. Please go through the guidelines, |
Hmm. |
@aimane-chnaif, how can i test ButtonWithDropdownMenu? (where is it used and how can i see the usage)? |
|
i tried overriding beta permission but as soon as I click on workspace chat it disappears, how can i get the beta permissions? |
Please ask permission on slack providing your emails to be added to beta. @PauloGasparSv might help as assigned engineer here |
thanks @aimane-chnaif, just one question if i ask for permission for [email protected]. all test accounts (eg."[email protected]") got the permission right? or should i ask for all the test email accounts? |
no, only required email |
okay thanks, and what are testing steps for AddPaymentMethodMenu in BaseKYCWall? |
Hey @ishpaul777 I found the slack post where you requested access. Sorry for not checking this myself but do you know what exact Betas you need?
Aimane already answered this but yeah, I'm pretty sure it doesn't work like that and that the betas will only be applied to the account you specifically ask them for! |
Thanks for your response @PauloGasparSv, can you add me to the betas that are required for testing of AddPaymentMethodMenu in BaseKYCWall and ButtonWithDropdownMenu. email - [email protected], [email protected], [email protected] |
Sure thing @ishpaul777! As mentioned on slack I have added your 3 accounts to the Please note that Betas will usually take up to an hour to have an effect. So if this doesn't work let's wait a bit before checking if you need another Beta or something : D |
I greatly appreciate your assistance, @PauloGasparSv. Could you kindly provide a response to #24284 (comment) |
@aimane-chnaif I tried to override the condition here https://github.com/Expensify/App/blob/043b1eded5eef81d06611df2891b0d6c793b42dd/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js#L290C1-L291C1 Notice a weired positioning of AddPaymentMethodMenu. https://www.loom.com/share/bb08c53218f44163a24fb1a39d23fae8?sid=46a8cc00-00b9-4545-876f-559454fffac6 |
🎯 ⚡️ Woah @aimane-chnaif / @ishpaul777, great job pushing this forwards! ⚡️ The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉
On to the next one 🚀 |
External issue reporter - @dhanashree-sawant - $250 - OFFER SENT |
Paid |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
App should close the modal when we click on 3 dots again like it does for all other type of modals throughout the app
Actual Result:
App does nothing when we click on 3 dots again
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: v1.3.51-0
Reproducible in staging?: Y
Reproducible in production?: N
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
Notes/Photos/Videos: Any additional supporting documentation
Recording.1134.mp4
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691491820472469
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: