refactor: separate BNPL methods from settings list #9268
Merged
Conversation
This file contains 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
Aug 15, 2024
Comment on lines
-232
to
-241
| <SettingsSection | ||
| description={ PaymentMethodsDescription } | ||
| id="payment-methods" | ||
| > | ||
| <LoadableSettingsSection numLines={ 60 }> | ||
| <ErrorBoundary> | ||
| <PaymentMethods /> | ||
| </ErrorBoundary> | ||
| </LoadableSettingsSection> | ||
| </SettingsSection> |
There was a problem hiding this comment.
Moved into the PaymentMethodsSection component.
|
Size Change: +1.19 kB (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
frosso
commented
Aug 15, 2024
| @@ -0,0 +1,205 @@ | |||
| /** @format */ | |||
There was a problem hiding this comment.
This is basically the old client/payment-methods/index.js component, but modified to accept a methodIds props - an array that represents the payment methods to render.
They could be BNPL methods on the good ol' UPE methods.
frosso
commented
Aug 15, 2024
| @@ -39,7 +39,7 @@ import CurrencyInformationForMethods from './currency-information-for-methods'; | |||
| import { getMissingCurrenciesTooltipMessage } from 'wcpay/multi-currency/missing-currencies-message'; | |||
| import { upeCapabilityStatuses, upeMethods } from '../constants'; | |||
| import paymentMethodsMap from '../../payment-methods-map'; | |||
| import ConfirmPaymentMethodActivationModal from 'wcpay/payment-methods/activation-modal'; | |||
There was a problem hiding this comment.
It's unfortunate that this add-payment-methods-task component needs to tap into another component's directory.
It was like this before, and it still is the case.
If I were to refactor it too, this would have been a much larger PR (on top of all the other changes that preceded it in #9261 #9265 #9255 #9262 #9260 ).
frosso
requested review from
a team and
gpressutto5
and removed request for
a team
6 tasks
|
re-assigning after talking with @gpressutto5 |
frosso
requested review from
a team and
brettshumaker
and removed request for
gpressutto5 and
a team
brettshumaker
approved these changes
Aug 16, 2024
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 #8248
Changes proposed in this Pull Request
Separating BNPLs into their own section in the settings.
They're already separate in onboarding, so this should be the only area they need to be separated.
The component rendering the list of payment methods (
client/payment-methods/index.js) has some specific logic to show a modal on deletion, show duplicate payment method information, and do some other business logic.I wanted to ensure we could render the payment methods in two section using the same component, without duplicating the logic.
So I made the
client/payment-methods/index.jscomponent "reusable" by passing a list of payment methods to render. I moved it toclient/settings/payment-methods-list/index.js. And I wrapped it by the newly createdBuyNowPayLaterSectionandPaymentMethodsSection.There's some files moved around (mainly everything from
client/payment-methodsmoved toclient/settings/payment-methods-list). So that's why there's lots of changes.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