Skip to content
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

Checkout: Hide edit button when only one payment method exists #90345

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

jjchrisdiehl
Copy link
Contributor

@jjchrisdiehl jjchrisdiehl commented May 7, 2024

This is a follow up PR to #90240

In #90240 we added the credit amount to the WordPressFreePurchaseSummary component to make the amount more obvious.

In this PR we are hiding the edit button for the Payment Method step if the step only contains one payment method - because editing a list of one isn't really possible.

Related to #88863

Before After
image image

Proposed Changes

  • Hide the stepper's HeaderEditButton for the Payment Method step when - 1) the paymentMethodStepIsActive 2) the paymentMethods array has a length of 1 or less

Testing Instructions

Regular purchases

  • Add a renewal to your shopping cart, ensure the cart has valid payment methods
  • Go to the Payment Method step and ensure that you see an edit link to change your payment method
  • Make sure you can change methods via the edit link

Credit purchases

  • Next, add enough credit to your account to cover the purchase completely
  • Refresh checkout and go to Payment method step, it should show WordPress.com Credits $x.xx
  • At this point there should not be an edit link next to the payment method step, but, there should be an edit link next to Billing Information
  • Try to find some combination that breaks this PR

@jjchrisdiehl jjchrisdiehl self-assigned this May 7, 2024
@matticbot
Copy link
Contributor

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~27 bytes added 📈 [gzipped])

name                          parsed_size           gzip_size
site-purchases                      +67 B  (+0.0%)      +27 B  (+0.0%)
purchases                           +67 B  (+0.0%)      +27 B  (+0.0%)
jetpack-cloud-partner-portal        +67 B  (+0.0%)      +27 B  (+0.0%)
checkout                            +67 B  (+0.0%)      +27 B  (+0.0%)
a8c-for-agencies-purchases          +67 B  (+0.0%)      +27 B  (+0.0%)

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 (~27 bytes added 📈 [gzipped])

name                                             parsed_size           gzip_size
async-load-calypso-my-sites-checkout-modal             +67 B  (+0.0%)      +27 B  (+0.0%)
async-load-calypso-blocks-editor-checkout-modal        +67 B  (+0.0%)      +27 B  (+0.0%)

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@jjchrisdiehl jjchrisdiehl added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 7, 2024
Copy link
Member

@sirbrillig sirbrillig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good idea and the code looks good to me. However, I have one major concern:

Remember that the "Edit" button doesn't just allow changing the payment method, it also allows editing the options of the active payment method.

Each payment method can have an active and an inactive UI. In the case of the free credits payment method, they are the same. However, some payment methods have quite a different for each version. For example, the credit card payment method shows a credit card form in its active UI but not in its inactive UI. With this change, I think it would become impossible to edit the credit card info if that was the only payment method available.

You probably actually want to only hide the edit button if the inactive UI is identical to the active UI, but this may turn out to be very hard to do since comparing component output is not easy.

What happens if a payment method doesn't have an inactive UI at all? Does it re-use the active UI? I can't remember. If so, maybe you could just remove the inactive UI from the credits payment method and hide the edit button if there is no inactive UI.

@jjchrisdiehl
Copy link
Contributor Author

Remember that the "Edit" button doesn't just allow changing the payment method, it also allows editing the options of the active payment method.

That is a good point 🤔

What happens if a payment method doesn't have an inactive UI at all? Does it re-use the active UI? I can't remember. If so, maybe you could just remove the inactive UI from the credits payment method and hide the edit button if there is no inactive UI.

It looks like if inactiveContent is undefined we would end up just showing nothing rather than falling through to the activeContent RadioButton component.

I wonder if it makes more sense to just explicitly set which payment methods should not show the edit button based on the ones we know show up by themselves?

@sirbrillig
Copy link
Member

sirbrillig commented May 9, 2024

I wonder if it makes more sense to just explicitly set which payment methods should not show the edit button based on the ones we know show up by themselves?

Yeah, that's one way to do it. Some sort of property on the payment method object maybe?

Either that or the suggestion I made above: re-use the activeContent if no inactiveContent exists and hide the edit button only then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants