-
Notifications
You must be signed in to change notification settings - Fork 69
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
Disable activation of LPM's requiring additional currencies when MC is disabled #7211
Disable activation of LPM's requiring additional currencies when MC is disabled #7211
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
Size Change: +335 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tpaksu We discussed this in Slack. I think the wording can be tweaked based on whether MC is required.
Also, the most important thing is that once merchants are in settings they have a clear indication of what to do next. We can use this design pattern that's consistent with other payment methods that need additional steps for setup. The error icon presents a tooltip on hover. Discussed in paJDYF-9Mc#comment-20802
@elizaan36 thanks for the info, so we'll be removing the global notice and adding warning icon with tooltips instead per LPM to fix this issue, right? |
hey @tpaksu I think the notice should remain in the modal to warn merchants about the situation. However, the settings pattern (warning icon with tooltip per LPM) is important as it's persistent once they've closed out of the modal. Based on the Slack thread, I think the language in the modal could be tweaked to something like this,
|
@elizaan36 they can't work together, as we'll be disabling selecting the LPM's with currency needs, and they won't be able to see the notice without being able to select them. And having them selected in onboarding, but not on the settings screen will be another issue. I suggest removing the notice and going with the checkbox -> warning icon replacement, and the tooltips. Or having them all checkable and showing the notice (as it is currently), without the tooltips. IMO, UX wise, letting them select the LPM's, showing the notice, suggesting them to enable the currency via a pill on the right side of the LPM title, stating something like |
This seems like the clearest pattern to indicate when a payment method requires more setup. Showing the checkbox indicates that the payment method can be enabled when in fact it can't. Let's go with this approach for now and I'll ping @rikmantel since he's taking a look at Multi Currency setup in the near future. Rik, could you take a closer look at this setup flow - the use case is when the Multi Currency extension has been deactivated, then the merchant tries to enable payment methods with WooPayments. |
@elizaan36 gotcha. Now to summarize what I understood:
And @rikmantel, welcome to the team! Looking forward to work with you! |
Sounds good to me. You could use copy similar to what I proposed for the notice for the tooltip.
|
@elizaan36 how does it look? |
Noting that in testing this that if Multi-Currency is enabled and a currency is not enabled, we automatically enable the currency without a note to the merchant if they are enabling the additional payment method outside of the wizard. I felt like I had come across this before, and I found where I asked about that here: p1684498606483579-slack-C01B8KNUYSW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Works as described. I added a comment in the PR itself, but that's unrelated to this code, as it's an issue that's present on develop
.
@jessepearson thanks for the review, I'll wait for @elizaan36's approval for merging this in. If Elizabeth approves it while I'm AFK, feel free to merge it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elizaan36 Sorry for the late reply here, this is what I am seeing through the Chome dev tools when tapping on the payment method. |
thanks for sharing @jessepearson! The tooltip looks off kilter. |
@elizaan36 possibly possible, but not on this PR :) |
@jessepearson on Chrome's developer tools, when I switch to mobile view, the arrows don't show up. Which browser did you use? It may also be something coming with the latest pushes. I merged the latest develop to this branch, and even on the desktop view, the arrows don't show anymore. Interesting. @elizaan36 will it be OK to push it like it is now? Settings screenLPM Onboarding |
Yes, this looks good! The arrow was removed in this PR when the mobile view was fixed. |
Fixes #7154
Changes proposed in this Pull Request
This PR implements the disabled checkboxes and tooltips feature for LPM's that require additional currencies that doesn't match with the store currency to operate, when the multi currency feature is turned off. The fix both applies the LPM activation box on Payments > Settings, and the LPM onboarding page.
Testing instructions
npm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge