search

Automattic / woocommerce-payments

Accept payments via credit card. Manage transactions within WordPress.
https://wordpress.org/plugins/woocommerce-payments/
Other
177 stars 69 forks source link

Ensure Button Settings are applied in ECE and Obtain Design Approval #8951

Closed rafaelzaleski closed 5 months ago

rafaelzaleski commented 5 months ago

Ensure that the settings for express checkout buttons in the WooPayments settings page are correctly applied to the Express Checkout Element (ECE) buttons. Additionally, secure final design approval for ECE buttons in both shortcode and blocks checkout.

### Tasks
- [x] Settings from WooPayments intended for PRBs are now appropriately applied to ECE buttons.
- [x] See if we can default to showing both GPay and APay together when available instead of showing a `See more` link to show both buttons.
- [ ] Design Review.
- [ ] Incorporate any feedback from the review and finalize the design.
bborman22 commented 5 months ago

Hey team! Please add your planning poker estimate with Zenhub @asumaran @cesarcosta99 @lovo-h @rafaelzaleski @reykjalin @ricardo

reykjalin commented 5 months ago

@elizaan36 @nikkivias @pierorocca I've added the styling options to the ECE buttons to see what they would look like. Can you take a look and see if there's something that we need to improve?

One thing that comes to mind is that we offer a Large variant at 56px height, but the maximum height supported by ECE is 55px (see below). There may also be some border radius differences we can now address with a new setting that's available for ECE.

Google Pay

[!NOTE] Google Pay does not have a "white outline" variant which is why it's not present in the examples here.

Blocks Shortcode
Black+ image image
White+ image image
Small (40px) image image
Large (55px)* image image

+ These are at the default (medium [48px]) height, which is why that size isn't part of the table. * 55px is the maximum height supported by ECE, even though our settings indicate 56px.

New options

We can now control the border radius, if we want, which I'm not sure we could do with PRBs.

Blocks Shortcode
Default border radius See button screenshots above See button screenshots above
0 border radius image image
99999px border radius image image
100% border radius image image

100% doesn't look quite right 🫠

Apple Pay

[!NOTE] I'm not sure why there's a bullet (•) in the WooPay button on Safari, but I'm going to look into that separately.

Blocks Shortcode
Black image image
White image image
White outline image image
Small (40px) image image
Large (55px) image image

New options

Blocks Shortcode
Default border radius See button screenshots above See button screenshots above
0 border radius image image
99999px border radius image image
100% border radius image image

100% border radius seems to behave better for the Apple Pay button for some reason 🤷‍♂️ . Or maybe just in Safari?

pierorocca commented 5 months ago

Corner radius is a huge win and one of many motivations to invest in ECE. @nikkivias is this something you'd recommend we implement in the current admin (preferred) or wait for the settings refresh?

Corner radius on top of the upcoming theme global settings support in WooPay would really propel WooPayments forward as the gateway with the best compatibility.

pierorocca commented 5 months ago

It's exciting to see this come to life. Thanks @reykjalin .

  1. Confirmed 55px is the maximum so we'll need to update the copy in the admin settings to reflect that max please.
  2. https://github.com/Automattic/woocommerce-payments/issues/8136 can we get that closed along with this effort?
  3. The * Apple is causing, is that also contributing to the large gap between the WooPay and Apple Pay buttons on Blocks?
  4. With ECE both Apple and Google Pay buttons should now be visible at the same time on supported browsers. Could we see that scenario please? This was also a factor for the move to ECE. I recall there was some code on the WooPayments side making it an either / or based on browser in addition to the Stripe PRB limitation. That needs to be removed if it hasn't.
  5. I'm seeing the ECE overflow "See more". Do we want to disable that so Stripe isn't on control of what to and not to show?
reykjalin commented 5 months ago

Confirmed 55px is the maximum so we'll need to update the copy in the admin settings to reflect that max please.

Got it 👍 . Added it to the list of tasks here.

https://github.com/Automattic/woocommerce-payments/issues/8136 can we get that closed along with this effort?

I don't think it fits in the same PR, but maybe I can pick that up in a separate PR? I'll see if it's an easy fix and do it now if that's the case, otherwise defer to @bborman22 on priority relative to other sprint tasks.

The * Apple is causing, is that also contributing to the large gap between the WooPay and Apple Pay buttons on Blocks?

Don't know. I'll report back once I've had time to look into that some more.

With ECE both Apple and Google Pay buttons should now be visible at the same time on supported browsers.

They are! That's what's below the See more thing (screenshot from Safari):

image

I'm seeing the ECE overflow "See more". Do we want to disable that so Stripe isn't on control of what to and not to show?

I'll see if we can change that, not sure if it's something we can affect though 🤔

pierorocca commented 5 months ago

I'll see if we can change that, not sure if it's something we can affect though 🤔

Thanks @reykjalin. It's a toggle option in their embedded component demo so I'd expect there to be a published or perhaps and unpublished attribute that controls overflow. If you can't readily find an option to control this, let's go ahead and pose the question to Stripe about how to disable overflow.

pierorocca commented 5 months ago

Found it. https://docs.stripe.com/js/elements_object/create_express_checkout_element#express_checkout_element_create-options-layout-overflow

reykjalin commented 5 months ago

Ok yeah, this is what it looks with overflow set to never:

Shortcode Blocks
image image
pierorocca commented 5 months ago

Interesting!!

OK 2 things:

  1. I reacted too quickly to the 55px and didn't think of the repercussions to other gateway's buttons sizes which have slowly been adhering to our design guidelines. Let me spin up a conversation with @nikkivias and Stripe.

  2. https://docs.stripe.com/js/elements_object/create_express_checkout_element#express_checkout_element_create-options-layout affords us some control of rows and columns. @nikkivias do you have a perspective on allowing Stripe to auto manage the layout or should we force 1 column?

reykjalin commented 5 months ago

The 2 column layout looks nice, but it might not be viable design-wise because we can't enforce minRows or minColumns, only maxRows and maxColumns. Just throwing that in here to make it clear the decision is between auto-layout (depends on the width of the container I'm guessing) or single column at this point.

reykjalin commented 5 months ago

@ricardo I think the width issue is a problem in Storefront. Stripe is doing some weird magic with their iframe that sets margin: -4px and then "makes up" the difference by setting width: calc( 100% + 8px ) (where the 8px make up for the negative margin on either side).

However, storefront sets max-width: 100% on iframes which means those 8px never get made up, causing the issue. If I remove the max-width style the button is fine:

https://github.com/Automattic/woocommerce-payments/assets/13835680/fd105a3b-546a-450a-bfe8-7d0b822e801c

We could fix this by adding our own max-width: calc( 100% + 8px ) !important on the button, but I wonder if that wouldn't make things difficult for other themes. I'm honestly not sure where to fix this problem 😕 . It might even be on Stripe to make sure the width isn't more than 100%. I'll admit they way they're going about the width of the button is odd and I have no idea why they do it this way.

pierorocca commented 5 months ago

Corrected above, I meant 1 column not 1 row and column. :) Does Stripe alter the width calc if we force a 1 column layout?

reykjalin commented 5 months ago

Does Stripe alter the width calc if we force a 1 column layout?

Nope.

The * Apple is causing, is that also contributing to the large gap between the WooPay and Apple Pay buttons on Blocks?

It is not caused by the same issue. I actually don't know where the gap is coming from. The browser dev tools don't reveal why the height of the whole container is off. I suspect it has something to do with the iframe from Stripe, but I can't figure out why. We may have to set an explicit height or somehow try to change the height once everything is rendered. It'd be very hacky.

As for the • I think it's just a Safari bug? Look at this video:

https://github.com/Automattic/woocommerce-payments/assets/13835680/e55260d6-deec-441b-a125-65993e6a501b

Disabling and re-enabling the list-style in the inspector causes the bullet to disappear but not reappear. I have noooo idea what's going on there. It'd be interesting to see if anyone else can reproduce this because I have no idea what could be going wrong here.

pierorocca commented 5 months ago

I brought up the column of 1 in case Stripe was performing some padding math dependent on number of rows and columns occupied by buttons.

The list property list-style: disc; where is that being inherited from or set? Could it be set to none or changed to list-style-type: none; in case Safari is having issues with the shorthand property?

reykjalin commented 5 months ago

he list property list-style: disc; where is that being inherited from or set?

It's from style.css, which comes from the theme installed on the site. I can try to add some !important style to the component CSS, but I'm not sure I can target the list element because the component is rendered inside the list element, we don't have direct access from it.

I can experiment with some CSS selectors though and see if I can fix it from within WooPayments, but otherwise this is really just a theme issue.

reykjalin commented 5 months ago

I've tried adding both list-style: none; and list-style-type: none; to the button itself and that doesn't work. So this is probably a combination of a theme issue and a bug in Safari/WebKit honestly.

reykjalin commented 5 months ago

I opened https://github.com/Automattic/woocommerce-payments/issues/9005#issue-2370898736 to deal with the incorrect button width. That seems out of scope for this issue.

I also removed the button height adjustment from this issue since that's probably best served being its own issue. I'll open that now.

reykjalin commented 5 months ago

Ok, opened https://github.com/Automattic/woocommerce-payments/issues/9006#issue-2370906128 to track the button height issue.

reykjalin commented 5 months ago

Now we just need design approval from @nikkivias and @elizaan36

elizaan36 commented 5 months ago

hey @reykjalin Thanks for this. I can confirm that the max height will need to be updated to 55px. This is also being updated as part of the Express Checkout API work that @alexflorisca is working on.

We'll update the design guidelines to reflect this change. Other than the issues you've already pointed out LGTM. cc @nikkivias In case you have additional comments.

pierorocca commented 5 months ago

I am double checking with Stripe if the 55px is a hard cap on the height and if there's a reason for 55px specifically. Let's hold off momentarily on updating the external spec please so we avoid any unnecessary 3P dev churn.

pierorocca commented 5 months ago

Stripe has responded. The 55px limit is based on PayPal's spec. In the PRB element, PayPal is absent hence the reason for this new limitation in the ECE. Appears we don't have a choice and will have to move. @aheckler fyi for documentation.

Piero Rocca - turns out that this is due to the restriction from PayPal (which wasn't available on PRB) - https://developer.paypal.com/sdk/js/reference/#link-size

reykjalin commented 5 months ago

I just ran into an interesting issue; under storefront the button isn't always displayed on the Checkout page because the Express Payment Methods container is too narrow:

image

The full error in the JS console is:

[Stripe.js] The parent container is not wide enough for Express Checkout Element to display the button types you chose. Instead, the element is showing 'plain' buttons. This may also cause the Google Pay button to disappear, so please adjust the window width to solve this issue.

@rafaelzaleski @pierorocca @elizaan36 @nikkivias this seems like a blocker for deploying ECE support in production since this would require some manual intervention from either theme authors or maybe a change in the Checkout block? I don't think this is something we can fix from the WooPayments side 🤔

Recording of the issue https://github.com/Automattic/woocommerce-payments/assets/13835680/ee934d61-f767-42f4-9a08-5cc37e3ea47a
pierorocca commented 5 months ago

Thanks @reykjalin. This is actually a problem with the PRB as well so this isn't a major blocker imo. Team Rubik is looking at no text by default for the Checkout page which would help with this issue by freeing up space. I also have a lower priority enhancement in backlog to eliminate any button text should the container width get too small. https://github.com/Automattic/woocommerce-payments/issues/8135

reykjalin commented 5 months ago

That's great @pierorocca , but not enough to fix the issue:

https://github.com/Automattic/woocommerce-payments/assets/13835680/5d61034d-62ec-41f8-901f-c51da52d84d8

This is using the icon-only buttons, and still the container can be too narrow. I think we probably need to enforce some minimum width for the container instead. That's not to say Rubik won't address that, but just removing text is no longer enough due to the way GPay adds text when it can while loading and doesn't remove it.

reykjalin commented 5 months ago

To demonstrate; this is not a problem when the GPay button is rendered with only the icon, such as in the case where both APay and GPay are available (i.e. in Safari):

https://github.com/Automattic/woocommerce-payments/assets/13835680/26ad456a-83d1-48c9-ba67-23e8a6511733

reykjalin commented 5 months ago

But I agree, not part of this issue 👍

pierorocca commented 5 months ago

When the viewport narrows to a mobile sized viewport, the button stack doesn't change to a vertical orientation. I think that's a limitation of the block. On shortcode we opted to stick with a vertical orientation, mainly because of compatibility issues. Definitely a forced compromise, but the buttons never disappear. Trying to maintain the horizontal orientation while the walls close in seems futile.

Here's how Stripe is handling it in the ECE demo: https://d.pr/i/M25QZt

Thoughts?

alexflorisca commented 5 months ago

Just scanned through this issue and wanted to add that we would need to render each button in a separate ECE instance in order to fit in with our current express checkout layout. We need to render multiple express payment methods side by side, in equal sized buttons, which we control via a grid layout. The new ECE layout doesn't work with this model, but I've reached out to Stripe who advised us to try rendering one button per ECE instance. This seems relevant to the design discussions here.

I'm also working on a proposal to sync express button settings like height and border-radius here which may impact this work

pierorocca commented 5 months ago

@reykjalin @c-shultz @alexflorisca thanks Alex. Should we connect to ensure that we're working towards the future state?

IIUC Blocks is maintaining a horizontal orientation. A natural breakpoint to a vertical stack is needed here. We cannot fully dictate the button text and some buttons the text is fixed. I'd not want to see a scenario where the Google Button or the Link Button is full width because that text is not alterable while Apple and WooPay are half the size and given less prominence. That's a no go for me.

reykjalin commented 5 months ago

we would need to render each button in a separate ECE instance in order to fit in with our current express checkout layout

We're going to be looking into that as part of this sprint in #8872 so we should at least be able to do some testing while working on that.

alexflorisca commented 5 months ago

We're going to be looking into that as part of this sprint in https://github.com/Automattic/woocommerce-payments/issues/8872 so we should at least be able to do some testing while working on that.

That's great, could you tag me in on the work you're doing there? It would be good to keep in sync to make sure we're working towards the same thing like Piero mentioned, and I may be able to help a little as I've played around with separate ECE instances briefly.

Can I just clarify if you're planning on releasing the ECE work before you investigate separate ECE instances? I think this is where I'm a little confused, as for me, it would be a must to render individual ECE instances for this. Which may also simplify the conversation a little bit

rafaelzaleski commented 5 months ago

Can I just clarify if you're planning on releasing the ECE work before you investigate separate ECE instances?

We’ll definitely look into this deeper before any ECE release to better understand how complex it might be. I'll start to work on #8872 this week.

alexflorisca commented 5 months ago

We’ll definitely look into this deeper before any ECE release to better understand how complex it might be. I'll start to work on #8872 this week.

Awesome, let me know how it goes!