-
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
Decouple './client/multi-currency-setup' #9348
Conversation
Sharing occurs between WooPayments v1 and MCCY v2.
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: +5.98 kB (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
As opposed to being imported from 'multi-currency/data'. So, we mock 'wcpay/data' in order to prevent the other store hooks/data from initializing.
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.
@lovo-h I understand this also depends on 107-gh-Automattic/woopayments before it's fully functional, but should I run any specific tests at this point?
Left just one comment but overall the changes make sense and LGTM.
CollapsibleBody, | ||
Search, | ||
LoadableBlock, | ||
} from 'multi-currency/interface/components'; | ||
import WizardTaskItem from '../../wizard/task-item'; |
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.
Question: It seems like we have a pretty similar WizardTaskItem
from interface/components
. I wonder if we should use only one? And adapt V1 to have a visibleDescription
prop.
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.
Good call out. De-duplicated in 1424558.
@ricardo: Actually, at this point, we can go ahead and test the multi-currency setup wizard:
By running these tests, I ran into an issue where the currencies were not displaying. This was fixed in 4996381. |
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 and tests well.
- ✅ Multi-currency setup tests.
- ✅ Files were moved to
/multi-currency/client/setup
.
Fixes 108-gh-Automattic/woopayments.
Changes proposed in this Pull Request
The changes in this PR continue the MCCY migration process to V2. More specifically, the MCCY setup is migrated to V2. However, this is an intermediary change that will depend on the changes from the following issues to be completed before it is fully functional:
Testing instructions
Confirm Full Migration of `./client/multi-currency-setup
npm run test:js
command and ensure all tests pass../client/multi-currency-setup
were moved to./multi-currency/setup
, properly disconnected from WooPayments, properly reconnected into MCCY, and interfaced with WooPayments through the interface later (e.g../multi-currency/interface/*
). Note: there exist some dependencies to some UI components in./client/multi-currency
that will need to be migrated. However, this cannot happen until after 107-gh-Automattic/woopayments has been merged.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