You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Payment - Preview is not shown when deleting paypal.me payment method #8097
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:
Identify the pull request that introduced this issue and revert it.
so it might be causing the anchorPositionTop to not be zero on desktop if there is only the paypal me option (I see in the Pr that the tests were usually with more options). cc @phivh would you be able to have a look whether you see any link between these?
Also not sure if this needs to be deployblocker if it also occurs in Production. Can users use Paypal.me now in N6? They cant use IOU yet.
@mountiny the changes that I did only kept the position of the menu while the window resize on the desktop which was the same logic to set position before. Nothing changed in the menu UI or show/hide logic.
// We want the position to be 13px to the right of the left border
anchorPositionLeft: position.left+13,
});
}
I thought the payment preview was only shown for the mobile site because the delete button on the desktop is right under the Paypal payment item.
Thus, I am quite sure that the change in the PR is not linked to this issue.
Activity
OSBotify commentedon Mar 11, 2022
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open
StagingDeployCashdeploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:MelvinBot commentedon Mar 11, 2022
Triggered auto assignment to @madmax330 (
Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.mountiny commentedon Mar 11, 2022
Except of the issue which is already linked (which I am not sure if is causing this problem), there is also this PR which made changes to this:
App/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js
Line 228 in d05c56e
so it might be causing the
anchorPositionTopto not be zero on desktop if there is only the paypal me option (I see in the Pr that the tests were usually with more options). cc @phivh would you be able to have a look whether you see any link between these?Also not sure if this needs to be deployblocker if it also occurs in Production. Can users use Paypal.me now in N6? They cant use IOU yet.
phivh commentedon Mar 13, 2022
@mountiny the changes that I did only kept the position of the menu while the window resize on the desktop which was the same logic to set position before. Nothing changed in the menu UI or show/hide logic.
App/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js
Lines 93 to 105 in d05c56e
I thought the payment preview was only shown for the mobile site because the delete button on the desktop is right under the Paypal payment item.
Thus, I am quite sure that the change in the PR is not linked to this issue.
mountiny commentedon Mar 13, 2022
@phivh Thank you very much for looking into this 🙇
MelvinBot commentedon Mar 14, 2022
Triggered auto assignment to @bfitzexpensify (
External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.3 remaining items
bfitzexpensify commentedon Mar 15, 2022
Upwork post here
MelvinBot commentedon Mar 15, 2022
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (
Exported)MelvinBot commentedon Mar 15, 2022
Triggered auto assignment to @marcaaron (
Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.thesahindia commentedon Mar 15, 2022
Proposal
We show the preview if
isPopoverBottomMountis true, if we wanna show the payment preview in all the cases then we can just remove this checkApp/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js
Lines 305 to 309 in 3696074
also we aren't using
isPopoverBottomMountanywhere else so I think we can also remove it from here -App/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js
Line 228 in 3696074
Santhosh-Sellavel commentedon Mar 15, 2022
@marcaaron

Showing preview would look redundant, as the popup is close to the selected payment method.
Can you confirm on this do we need this change?
Santhosh-Sellavel commentedon Mar 15, 2022
@marcaaron
If we need this change we are good to go with @thesahindia proposal #8097 (comment)
🎀👀🎀 C+ reviewed
marcaaron commentedon Mar 18, 2022
I don't really get this issue at all or understand why we need a preview for this. Seems extremely low value to me.
marcaaron commentedon Mar 18, 2022
@madmax330 says
I think it should be exactly how it is now. If anyone disagrees feel free to reopen.