-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Billing: Replace PaymentMethodForm
in add card form
#48968
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~84 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~1390 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~126 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
It's derived from the purchase and we're already passing in the purchase.
Previously saveCreditCard did not accept formFieldValues (country and postal code); this adds that option. Secondly this makes the function call the API endpoint directly rather than having the call injected as an argument. This better matches the nearby updateCreditCard method and should help to remove some confusion about how saveCreditCard works.
5d0cf23
to
994f437
Compare
I split up most of this PR into smaller pieces:
Some pieces are not included in the above, specifically the updates to |
Closing as redundant to the other PRs. |
Changes proposed in this Pull Request
This replaces
PaymentMethodForm
withPaymentMethodSelector
(the updated payment method selector that's already being used byChangePaymentMethod
) inAddPaymentMethod
and both site-level and account-levelAddNewPaymentMethod
components.See: #48966
Some of these changes are not directly related to the "add card" forms so this could be broken up into multiple PRs.
To do
PaymentMethodSelector
finishes, it displays its own notice. This notice is not as accurate as the notices already displayed by the functions inclient/me/purchases/components/payment-method-form/helpers.js
, and since you cannot display more than one notice at once, the new notice overwrites the old one. It would be better if the notice was displayed only by the component which adds or updates the payment method rather than the function that does the update. Changing this will require modifying the existing functions. Which leads us to...client/me/purchases/components/payment-method-form
which is the directory for thePaymentMethodForm
component that is being deprecated by this PR. We should copy those functions elsewhere so that the existing ones can eventually be removed. (There are similar functions inclient/state/stored-cards/actions.js
but those are for Redux only.)AddNewPaymentMethod
but we need it to work for the site-level one as well.AddPaymentMethod
but I haven't tested it because I can't remember how to get that form to be displayed. Do we really need a separate one?PaymentMethodSelector
to add a new payment method, it updates all existing purchases to use that payment method; this is terrible UX and we'll need to handle that somehow (this is a known issue but we must not forget about it in this PR even if we plan to change the behavior elsewhere).PaymentMethodForm
in the Woo shipping labels section. That's probably fine but we have to make sure we don't break that accidentally by these changes.Testing instructions
WIP