Integrate express payments buttonAttributes API from the Checkout Block #8888
Conversation
alexflorisca
added
component: checkout
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. |
|
Size Change: +487 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
client/checkout/woopay/express-button/woopay-express-checkout-button.js
Outdated
Show resolved
Hide resolved
…button.js Co-authored-by: Thomas Roberts <[email protected]>
alexflorisca
force-pushed
the
try/poc-express-checkout-button-styles
branch
from
83c2d95
to
d0951e5
Compare
alexflorisca
force-pushed
the
try/poc-express-checkout-button-styles
branch
from
066ccc5
to
ca158f0
Compare
alexflorisca
changed the title
|
@alexflorisca is there an issue for this change? This PR looks like it slipped through the cracks. We use issues and focus area labels to assign things to teams (and get stuff merged ⇒ shipped). @c-shultz it looks like this is front end / checkout related, could someone in shopper experience group take charge of this, e.g. review and go from there? FYI @vbelolapotkov – do we have a process for catching external contributor PRs like this one (from other teams in Woo, or external)? |
There was a problem hiding this comment.
This isn't testing very well for me. The Google Pay and Apple Pay ECE buttons don't render for me at all in this PR even though they do on develop. I'm honestly not sure why that is. There are no relevant errors in the browser console that I can see, and nothing obviously wrong in the code. And yet they don't render for me. I tried to track the issue down but couldn't figure it out.
Then there is the preview in the Cart Block while adjusting the button radius. That's something that needs to be fixed before we even consider releasing this in WC Core IMO:
Screen.Recording.2024-08-23.at.5.12.27.PM.mov
The border radius previews are incredibly wonky and don't provide an accurate representation of what the buttons will look like. Not mention that all three of them are different.
|
PS Github needs a broader, funnier set of emojis. ⬜⬜⬜⬜⬜⬜⬜⬜🟧🟧🟧🟧🟧🟧🟧🟧⬜⬜⬜⬜⬜⬜⬜⬜ |
|
Thanks for the review @reykjalin.
I'm not sure what's going on there, they seem to render ok for me with this branch 🤷 Can you test against the
Agreed. This was an interim solution while working on rendering the actual buttons in the editor. At the moment, a lot of payment methods render images here, and the border radius is applied to the containing element. I'm starting work on woocommerce/woocommerce#42353, which will solve this issue. This shouldn't require any more changes from WooPayments (maybe deprecating a prop, but that's it). So this improvement will come next, but I don't think it should be a blocker for this PR. It would be great to get this work out there before the work in Core is released. |
That's the branch I was testing against 🙂
I appreciate that point of view, but I disagree 😕 . We've struggled with maintaining quality across our extensions and I don't think we should release this in WooPayments until the feature looks and feels good across the entire experience. Until it does I'd recommend we don't release this. I know that runs the risk of making this PR stale, but I don't want us to release anything here until all the issues have been fleshed out. I'm going to have really low availability for all of September so I don't think I can keep testing and reviewing the PR. @bborman22 is there someone else from Fractal that could take over the reviews here in my place? |
@rafaelzaleski would you be able to take over the review for this PR in @reykjalin's absence? We can catch up on this on Slack as needed. |
I appreciate what you're saying here, quality is important, and I wouldn't want to ship something broken. First and foremost, I'd like to make sure that everything in this PR works with the current WC release (without the express checkout work in core). I think that's key to releasing this in Woo Payments. Secondly, I acknowledge. the difficulty in testing something that relies on parallel ongoing work. My plan was and still is to split this work into 2 parts in Core. The first part is done now, and it was around introducing the controls and tidying up the UX. The second is rendering the actual buttons in the editor, to better reflect the shopper's experience. That's ongoing now. So I'd love for this PR to be tested with that in mind. I can create an issue to do some more testing once the second part of the work has been done in Core. If there any issues related to the visual appearance of the buttons, or the controls not working, then we should address those before merging this. If the only concern is that the border radius controls are mis-represented in the editor, because they are applied to ALL buttons, I'd ask that we take into account that this won't be the case very soon. Thanks for the healthy discussion around this, appreciate your efforts here and I'd like to work on finding a good solution to move forward with this. |
There was a problem hiding this comment.
There’s an issue with the WooPay button’s font size, and according to step 8 of the testing instructions:
- Notice both the height and the border radius are applied to the WooPay button, and only the height is applied to the payment request method.
However, the border radius is also being applied to the ECE buttons. I see you added a notice in the WooPayments settings, which I think is great to avoid confusion. I just wanted to mention this to confirm that this is the expected behavior.
Other than that, everything seems to be working great!
Blockers:
- Fix the WooPay button font-size issue (it's very noticeable in Safari).
- Do not show the notice in settings if it's not relevant (no cart or checkout blocks).
- Render real ECE buttons in the preview (I'm ok with addressing this in a new PR).
| <a href={ checkoutPageUrl }> | ||
| { __( 'Cart & Checkout blocks.' ) } | ||
| </a> |
There was a problem hiding this comment.
I think we should add a checkoutPageUrl link to the word "checkout" and a cartPageUrl link to the word "cart." Also, we shouldn't render this notice at all if both are empty.
There was a problem hiding this comment.
I had that to start with but I chose to only link to Checkout block for 2 reasons:
- Both include the same express payment controls
- Keep it simple for the merchant and give only 1 option
- It gets real messy with translations having to translate
Cart,and,Checkout,blockseparately, because of the links.
I think this is ok, but if you feel strongly about this I'm sure we can figure something out.
|
Thanks for the review @rafaelzaleski. I've left a comment regarding the Cart link and I've put the
I'm not seeing the font go quite as small as your screenshot there, so I wonder if you can provide more details about this? I can see the text change when changing size, but I think that's expected behaviour right? Or maybe adding the data-size back in fixed it? If you can give this another go and let me know that'd be great. Screen.Recording.2024-09-06.at.17.15.42.mov
As far as I know, there's no client side flag to check if the default cart & checkout pages are block based or not. This is something that would be useful to have though, but it needs a change in Core. I can add an issue to both Core and here to make sure this gets done, but it might stall this PR a while. I think it's a nice to have optimisation, but I think we can move forward with this PR without it. What do you think?
Yes, I meant the actual PRB button, not the ECE ones. Which I think are the default ones now so that's ok. |
There was a problem hiding this comment.
Thank you for the changes. The font size of the WooPay button is working correctly again after re-adding the data-size attribute.
As far as I know, there's no client side flag to check if the default cart & checkout pages are block based or not. This is something that would be useful to have though, but it needs a change in Core. I can add an issue to both Core and here to make sure this gets done, but it might stall this PR a while. I think it's a nice to have optimisation, but I think we can move forward with this PR without it. What do you think?
I think that's fine, but we should remove the link from this notice. On my test site, clicking it redirects to the shortcode checkout. If we can't ensure the link points to the page where the settings might be overridden, it's better not to include a link.
|
Thanks @rafaelzaleski, I've removed the link from the notice for now. |
There was a problem hiding this comment.
I’ve tested the PR again, and everything seems to be working well. Thank you for taking the first step in integrating with the buttonAttributes API!
✅ The WooPay font-size issue has been resolved.
✅ The height and border-radius attributes are now passed from the editor to the buttons in the cart & checkout blocks, overriding the plugin settings, while the plugin settings are still applied on the shortcode cart & checkout pages.
✅ There are no regressions if the buttonAttributes API is not available.
Issues to be addressed in another PR:
- [ECE] Replace SVGs with ECE buttons in the Blocks Editor preview #9417
- Ongoing discussions about dark mode and button colors?
|
Thank you for approving it. Seems that it's still hung up on @reykjalin's review needing changes so it's not giving me the green light to merge. Given you took over the review, I think it's ok to merge as is? |
Changes proposed in this Pull Request:
This PR uses the
buttonAttributesAPI introduced in Woo Core in Add new buttonAttributes API to style express checkout buttons coherently. This will respect the newheightandborderRadiuscontrols on the express checkout block, overriding extension settings when these are available. It also uses thedarkModesetting to override the button theme.This means the woo payments specific settings are still applied if we're not in a checkout block context. But if we are, the button sizes and colours will be synced across all express payment buttons that have integrated with this API.
Testing instructions
50pxnpm 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