update: MCCY settings with unbundled wp components #10930
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. |
frosso
commented
Jul 14, 2025
|
|
||
| const MultiCurrencySettingsPage = () => { | ||
| const [ |
There was a problem hiding this comment.
The previous implementation was using two useState: one for isSingleCurrencyScreenOpen, and another for currencyCodeToShowSettingsFor.
But we can only use currencyCodeToShowSettingsFor, isSingleCurrencyScreenOpen is basically a "falsy" version of currencyCodeToShowSettingsFor.
Having two state values where one can be inferred from the other seems wasteful, and can create sync issues (where there's the potential that only one of them updates).
frosso
commented
Jul 14, 2025
| <h2>{ __( 'Enabled currencies', 'woocommerce-payments' ) }</h2> | ||
| <p> | ||
| { createInterpolateElement( | ||
| sprintf( |
There was a problem hiding this comment.
No need to use sprintf - there is no placeholder
frosso
commented
Jul 14, 2025
| { | ||
| learnMoreLink: ( | ||
| // eslint-disable-next-line jsx-a11y/anchor-has-content | ||
| <a |
There was a problem hiding this comment.
The previous link was just using a a component, but opening in a new page.
Ensuring that we use the ExternalLink component, which adds the "Opens in a new tab" text for a11y.
frosso
commented
Jul 14, 2025
| @@ -81,7 +64,7 @@ const EnabledCurrencies = () => { | |||
| return ( | |||
| <SettingsSection | |||
| description={ EnabledCurrenciesSettingsDescription } | |||
| className={ 'multi-currency-settings-enabled-currencies-section' } | |||
There was a problem hiding this comment.
I noticed an inconsistent usage of braces for strings in this file - consolidating
frosso
commented
Jul 14, 2025
| @@ -113,7 +95,7 @@ const EnabledCurrencies = () => { | |||
| <EnabledCurrenciesListItemPlaceholder | |||
| key={ 'loadable-placeholder-' + i } | |||
| isLoading={ 1 } | |||
| ></EnabledCurrenciesListItemPlaceholder> | |||
There was a problem hiding this comment.
Updated the component usage to be self-closing, since there's no children.
frosso
commented
Jul 14, 2025
| @@ -100,7 +83,6 @@ const EnabledCurrencies = () => { | |||
| <EnabledCurrenciesListItem | |||
| key={ enabledCurrencies[ code ].id } | |||
| currency={ enabledCurrencies[ code ] } | |||
| defaultCurrency={ defaultCurrency } | |||
There was a problem hiding this comment.
defaultCurrency can be gathered by the EnabledCurrenciesListItem component without prop-drilling
frosso
commented
Jul 14, 2025
| @@ -20,29 +20,17 @@ const EnabledCurrenciesListItemPlaceholder = ( { isLoading } ) => { | |||
| </LoadableBlock> | |||
| </div> | |||
| <div className="enabled-currency__label"> | |||
| <LoadableBlock | |||
There was a problem hiding this comment.
Updated the component usage to be self-closing, since there's no children.
frosso
commented
Jul 14, 2025
Comment on lines
-23
to
-24
| // TODO: This works when saving, but list does not refresh. | ||
| // TODO: Should we reset selected currencies on modal close? |
There was a problem hiding this comment.
I couldn't reproduce this - cleaning up
frosso
commented
Jul 14, 2025
| { | ||
| learnMoreLink: ( | ||
| // eslint-disable-next-line jsx-a11y/anchor-has-content | ||
| <a |
There was a problem hiding this comment.
The previous link was just using a a component, but opening in a new page.
Ensuring that we use the ExternalLink component, which adds the "Opens in a new tab" text for a11y.
|
Size Change: +5.2 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
frosso
commented
Jul 14, 2025
| 'woocommerce-payments' | ||
| ) } | ||
| /> | ||
| <div className="multi-currency-settings__description"> |
There was a problem hiding this comment.
Removed this multi-currency-settings__description in favor of the help prop on the CheckboxControl, to be consistent with the other settings pages
frosso
commented
Jul 14, 2025
| @@ -1,14 +0,0 @@ | |||
| .settings-section__controls { | |||
frosso
commented
Jul 14, 2025
|
|
||
| const calculateCurrencyConversion = useCallback( |
There was a problem hiding this comment.
This component was using a double state - where calculatedValue was dependent on baseValue, and always updated whenever baseValue changed.
Clearly, calculatedValue is just a differently formatted baseValue (with a prefix/suffix and an updated formatting).
This can create sync issues. Ensuring that calculateCurrencyConversion is the calculated value that is rendered in the UI based off baseValue
frosso
commented
Jul 14, 2025
| <Card> | ||
| <CardBody> | ||
| <div> | ||
| <h4>{ storeCurrency.name }</h4> |
There was a problem hiding this comment.
These h4s were used as labels for the input - updating them to be actual labels
frosso
commented
Jul 14, 2025
| @@ -116,21 +132,6 @@ const SingleCurrencySettings = () => { | |||
| moment.unix( targetCurrency.last_updated ).toISOString() | |||
| ) | |||
| : ''; | |||
|
|
|||
| const CurrencySettingsDescription = () => ( | |||
There was a problem hiding this comment.
This is a component - it shouldn't be declared within the SingleCurrencySettings method's body. It's a performance issue within React.
frosso
commented
Jul 14, 2025
| } | ||
| > | ||
| <CardBody> | ||
| <h3> |
There was a problem hiding this comment.
Updating these to be in line with the main settings pages
frosso
commented
Jul 14, 2025
| 'Manual Rate', | ||
| 'woocommerce-payments' | ||
| ) } | ||
| </h4> | ||
| <input |
There was a problem hiding this comment.
Updating these to be TextControl from WP components
frosso
commented
Jul 14, 2025
| 'Price rounding', | ||
| 'woocommerce-payments' | ||
| ) } | ||
| </h4> | ||
| { /* eslint-disable jsx-a11y/no-onchange */ } | ||
| <select |
There was a problem hiding this comment.
Updating these to be SelectControl from WP components
There was a problem hiding this comment.
Pull Request Overview
This PR updates the multi-currency settings UI to use unbundled WordPress components, refactors styling and state handling for consistency with the main WooPayments settings, and refreshes related tests and snapshots.
- Replace bundled WP component wrappers with external SelectControl, TextControl, ExternalLink, etc.
- Remove custom SCSS form styles in favor of WP component styling and add
__nextHasNoMarginBottomsupport in the wrapper. - Refactor context and navigation to toggle views via
currencyCodeToShowSettingsFor, and update tests/snapshots accordingly.
Reviewed Changes
Copilot reviewed 22 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| includes/multi-currency/client/setup/tasks/store-settings-task/index.js | Added __nextHasNoMarginBottom prop and standardized className |
| includes/multi-currency/client/settings/single-currency/index.js | Swapped in WP SelectControl/TextControl, extracted descriptions |
| includes/multi-currency/client/settings/single-currency/style.scss | Removed bespoke form styles, adjusted margin rules |
| includes/multi-currency/client/settings/single-currency/currency-preview.js | Simplified conversion with useMemo, unified inputs |
| includes/multi-currency/client/settings/multi-currency/store-settings/index.js | Merged description into help prop, removed custom SCSS import |
| includes/multi-currency/client/settings/multi-currency/enabled-currencies-list/* | Refactored default currency hook usage, introduced ExternalLink |
| includes/multi-currency/client/index.js | Wrapped root in UnbundledWpComponentsProvider, simplified context |
| includes/multi-currency/MultiCurrency.php | Added wp-components as script dependency |
| client/components/wp-components-wrapped/make-wrapped-component.tsx | Extended prop types to include __nextHasNoMarginBottom |
| Tests & Snapshots | Updated snapshots and assertions to match new component APIs |
Comments suppressed due to low confidence (3)
client/components/wp-components-wrapped/make-wrapped-component.tsx:26
- [nitpick] Consider documenting the
__nextHasNoMarginBottomprop in the TypeScript interface or a README to clearly explain its purpose and usage for future maintainers.
// extracting the `__nextHasNoMarginBottom` from the list of props to avoid issues when running tests.
includes/multi-currency/client/settings/single-currency/index.js:194
- The heading level under the CardBody was changed from
to
; please confirm this maintains a correct semantic outline for screen readers and document structure.
<h4>
includes/multi-currency/client/settings/multi-currency/enabled-currencies-list/list-item.js
Show resolved
Hide resolved
rtio
approved these changes
Jul 14, 2025
I noticed that, too, but I thought it was a glitch. I now see that it happens almost every time. Taking a look, now! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes WOOPMNT-5120
Changes proposed in this Pull Request
Updating the components in the MCCY settings to use the WP components that come with the WP installation, rather than the WP components we bundle.
There might be some visual differences (with spacing or uppercase labels or with inputs being full-width), but that's how the components are rendered.
I took the opportunity for consolidating some of the UI to look and act more like the "main" WooPayments settings page: using WP components almost everywhere (I think the only one left out is the radio control in the currency edit screen); I ensured the styles are consistent with the main settings page.
The previous approach was not semantically correct not accessible: the labels for some of the inputs were
h4s and didn't have a related input, so the inputs appeared without label. Moreover, the help text was not associated with the input.I also took the opportunity for consolidating some of the internal states, which could benefit from a little refactor.
I did not change the UI to have the white background like the main settings screen - I'm not sure why it wasn't done before, so leaving as-is.
There is a difference in bundle size due to the import of the WP input components. But it won't be a problem once we unbundle them:
Before:
After:
Testing instructions
npm run changelogto add a changelog file, choosepatchto 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