feat(modal-checkout): update thank you page with new newspack ui #1649

chickenn00dle · 2024-01-23T18:44:55Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Part of 1206197591054328/1206405274735557/f

This applies the newspack ui to the modal checkout thank you page and prepares the rest of the modal checkout to be updated to the new ui. As a consequence this breaks the styling of the modal checkout and billing forms a bit, but I will work on those in a follow up PR so this one doesn't get too big:

Checkout form is expected to look like this:

Billing info form is expected to look like this:

How to test the changes in this Pull Request:

With newspack plugin on feat/update-checkout-block-thankyou-ui branch, trigger modal checkout via donate button
Complete checkout until you see the thank you page
Confirm the thankyou modal looks as expected with the udpated ui

Switch to master on newspack plugin and repeat the above steps
Confirm nothing appears to be broken (The thank you page should not have the new styles applied and just be a generic transaction summary)

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully ran tests with your changes locally?

chickenn00dle · 2024-01-25T19:47:13Z

includes/class-modal-checkout.php

-				<section class="newspack-blocks-modal__content">
-					<div class="newspack-blocks-modal__spinner">
+				<section class="newspack-ui__modal__content">
+					<div class="newspack-ui__spinner">


I couldn't find a place for a loading state in the main newspack ui changes, so moved the logic for this in Automattic/newspack-plugin#2878. So that will need to be checked out to ensure the spinner correctly functions while testing.

chickenn00dle · 2024-01-25T21:51:00Z

src/modal-checkout/modal.js

 				if ( window.newspackReaderActivation?.overlays ) {
 					modalCheckout.overlayId = window.newspackReaderActivation?.overlays.add();
 				}
 				iframeResizeObserver = new ResizeObserver( entries => {
 					if ( ! entries || ! entries.length ) {
 						return;
 					}
-					const contentRect = entries[ 0 ].contentRect;
-					if ( contentRect ) {
-						modalContent.style.height = contentRect.top + contentRect.bottom + 'px';


I was running into a strange bug where the iframe body was not the same height as the iframe document itself which was causing some issues with scrollbars. I got around this by getting the height of the iframe via offsetHeight instead which seems to have addressed the problem. Not sure if anyone has seen this issue before or if this is the best approach.

This is happening because the __content is applying a padding that, in the case of the modal checkout, is expected to live inside the iframe body (here), so the top and bottom values it picks up is not including the padding generated by parent element.

I'm not sure which strategy is best, but I remember having issues with the offsetHeight providing a consistent value across different stages of checkout/iframe changes.

I've suggested that we add the newspack-ui__modal__content CSS class to the <body> tag inside of the iframe further up in the review. This ends up adding more padding inside of the iframe, but I think it could help with this: we could use CSS to get rid of the problem spacing around the iframe, so the padding's all handled inside.

Something like this in Newspack UI could work if we can all iframes should act like this:

.newspack-ui__modal__content iframe { width: calc( 100% + var( --newspack-ui-spacer-8, 48px ) ); max-width: unset; height: calc( 100% + var( --newspack-ui-spacer-8, 48px ) ); border: none; margin: calc( -1 * var( --newspack-ui-spacer-5, 24px ) ); }

... or if it's specific to the modal checkout, we can add it to the iframe[name="newspack_modal_checkout_iframe"] selector in the blocks instead.

I did a rough test with the original JS and it seems to work! Let me know what you think 🙂

Nice! Thanks for the clarity and suggestions y'all 🙌

I was able to apply these styles and revert the iframe resizing logic in c8b0329

This looks good overall!

I did notice the spinner's no longer displaying -- it's using the Newspack UI classes but spinner styles don't exist there.

Similar to this comment, this could be something that just lives in the checkout modal, I think the CSS classes just need to be updated in the markup to not switch to the newspack-ui__ classes, and the CSS itself might need a bit of a massage so the structure still works with how the modal might be structured with the newspack-ui classes 🙂

I did notice the spinner's no longer displaying -- it's using the Newspack UI classes but spinner styles don't exist there.

I've actually moved the styles for the spinner over to newspack-ui in Automattic/newspack-plugin#2878. Sorry if it wasn't clear, but you'll need to check that branch out in the main Newspack plugin before testing this to see the spinner in action!

Ah thanks! I'm afraid I haven't been following things as closely as I should this week. When I have that branch checked out too, the spinner is there! 🙂

chickenn00dle · 2024-01-25T21:52:18Z

src/modal-checkout/modal.js

+					const iframeHeight = iframe.contentDocument.documentElement.offsetHeight;
+					if ( iframeHeight ) {
+						// Account for newspack ui modal content padding.
+						modalContent.style.height = iframeHeight + 48 + 'px';


Just noting that to get the iframe to load without a scollbar and to get the modal to load the iframe with the correct height, I had to account for the padding of the modal content here.

laurelfulford

Thanks for digging into this, @Raz! 🙂 I think it's on the right track overall! I left some comments/feedback -- please let me know if you have any questions at all about what I wrote!

laurelfulford · 2024-01-26T01:50:31Z

includes/class-modal-checkout.php

@@ -491,10 +505,10 @@ public static function get_checkout_template( $template ) {
 			<link rel="profile" href="https://gmpg.org/xfn/11" />
 			<?php wp_head(); ?>
 		</head>
-		<body id="newspack_modal_checkout">
+		<body class="newspack-ui" id="newspack_modal_checkout">


Suggested change

<body class="newspack-ui" id="newspack_modal_checkout">

<body class="newspack-ui newspack-ui-modal__content" id="newspack_modal_checkout">

I think we'll need a modal-specific class here, too, so any modal-specific styles will be picked up by these iframes.

(One specific example is that we'll be changing the button colour inside of the modal from black to something from the site's customizations).

Right now the Newspack UI styles don't nest content-specific styles under the __content class, but I can spin up a PR to move styles like this into the .newspack-ui__modal__content selector, and we can build those styles out from there!

Ah. Makes sense! I've added the modal specific class in c8b0329

laurelfulford · 2024-01-26T02:03:30Z

src/modal-checkout/modal.js

 				if ( window.newspackReaderActivation?.overlays ) {
 					modalCheckout.overlayId = window.newspackReaderActivation?.overlays.add();
 				}
 				iframeResizeObserver = new ResizeObserver( entries => {
 					if ( ! entries || ! entries.length ) {
 						return;
 					}
-					const contentRect = entries[ 0 ].contentRect;
-					if ( contentRect ) {
-						modalContent.style.height = contentRect.top + contentRect.bottom + 'px';


I've suggested that we add the newspack-ui__modal__content CSS class to the <body> tag inside of the iframe further up in the review. This ends up adding more padding inside of the iframe, but I think it could help with this: we could use CSS to get rid of the problem spacing around the iframe, so the padding's all handled inside.

Something like this in Newspack UI could work if we can all iframes should act like this:

.newspack-ui__modal__content iframe { width: calc( 100% + var( --newspack-ui-spacer-8, 48px ) ); max-width: unset; height: calc( 100% + var( --newspack-ui-spacer-8, 48px ) ); border: none; margin: calc( -1 * var( --newspack-ui-spacer-5, 24px ) ); }

... or if it's specific to the modal checkout, we can add it to the iframe[name="newspack_modal_checkout_iframe"] selector in the blocks instead.

I did a rough test with the original JS and it seems to work! Let me know what you think 🙂

laurelfulford · 2024-01-26T02:11:21Z

src/modal-checkout/templates/thankyou.php

+	<?php if ( $is_success ) : ?>
+		<div class="newspack-ui__box newspack-ui__box--success newspack-ui__box__text-center">
+			<span class="newspack-ui__icon newspack-ui__icon--success">
+				<?php // TODO: How should we handle sharing icons across plugins? ?>


Ooooh, good point! 🤔

I think at least these SVGs are just decorative; we might be able to use only CSS for them -- something like:

.newspack-ui__icon:after { background-color: currentcolor; content: ''; display: block; height: 24px; mask-size: 24px; width: 24px; } .newspack-ui__icon--success::after { mask: url('../path/to/svg/asset.svg') 0 0 no-repeat; }

We'd still need to duplicate must-have SVGs (like the close icon), so this might not be an ideal replacement!

Gotcha! I'm gonna leave this one as is for now and maybe we can tackle this in a follow-up PR?

That makes sense to me! 👍

laurelfulford · 2024-01-26T02:12:36Z

includes/class-modal-checkout.php

+
+		if ( class_exists( '\Newspack\Newspack_UI' ) ) {
+			$class_prefix = 'newspack-ui';
+		}


This is a nice approach to swapping the classes!

laurelfulford · 2024-01-26T02:44:04Z

src/modal-checkout/modal.js

@@ -129,7 +139,9 @@ domReady( () => {
 			form.addEventListener( 'submit', ev => {
 				const formData = new FormData( form );
 				// Clear any open variation modal.
-				variationModals.forEach( variationModal => variationModal.classList.remove( 'open' ) );
+				variationModals.forEach( variationModal => {


The variations modal isn't quite marked up like the modal checkout (variations vs. modal checkout), so changing the JS for it causes the Checkout Button to stop opening if it's for a variable product.

(To recreate, you need to have a variable product in Woo, and set up the Checkout Button block to use it, and check "Allow the reader to select the variation before checkout" -- here's a little more about the block and that specific feature!).

When I roll back to the epic/ras-acc branch the button still works. I think it should ultimately all be updated (with the same markup for the modal container and overlay), but that can be done separately 🙂 (if you like, I'm happy to take a swing at it!).

Ah. Good catch! I meant to ask about how to trigger this modal before touching the code for it, but it completely slipped my mind. Thanks for providing steps to recreate!

I was able to at least get the variation modal working again with and without newspack ui present in 3f03cf3

I wasn't able to find any newspack-ui classes for the select and option elements of the variation modal though. Does this exist already? Regardless, if it's okay with you, maybe we can tackle that in the next PR where I address the main checkout form?

The modal works like a charm now, thanks! 🙌

Does this exist already?

Not yet! I made an Asana task for this here, just so we have it tracked somewhere: 1205234045751551-as-1206482127342706

I'm happy to take a swing at it once I'm back to a regular work schedule if that would help!

For now, I've gone ahead and added the existing variation styles to newspack-ui in Automattic/newspack-plugin#2878, but if they need to be updated, then it'd be great if you could take a crack at it!

Oh nice -- thanks @chickenn00dle! 🙌

chickenn00dle · 2024-01-26T17:56:30Z

src/modal-checkout/modal.scss

-	100% {
-		transform: rotate( 360deg );
+// Checkout modal specific styles.
+.newspack-ui {


I decided to keep the iframe specific styles here rather than in the main newspack ui library. My thought is we should let each iframe dictate how it should be styled rather than enforce styles top down. That said, I don't feel too strongly about this, and am happy to move this over to the newspack ui library anyway.

adekbadek · 2024-01-30T10:53:36Z

@chickenn00dle –

As a consequence this breaks the styling of the modal checkout form a bit, but I will work on that in a follow up PR so this one doesn't get too big.

Can you please add a list of these breakages to PR description? I've seen this one when testing, not sure if it's expected:

includes/class-modal-checkout.php

adekbadek

LGTM, but I'll defer to @laurelfulford as the primary reviewer.

adekbadek · 2024-01-30T10:56:14Z

src/modal-checkout/modal.js

@@ -25,47 +27,54 @@ function domReady( callback ) {
 	document.addEventListener( 'DOMContentLoaded', callback );
 }

-const triggers =
-	'.wpbnbd.wpbnbd--platform-wc,.wp-block-newspack-blocks-checkout-button,.newspack-blocks-variation-modal';
+const CLASSNAME_BASE = newspackBlocksModal.has_newspack_ui ? 'newspack-ui' : 'newspack-blocks';


To avoid duplication, I'd suggest passing the value of Modal_Checkout::get_class_prefix() directly.

Good call! Updated in e188f8f

chickenn00dle · 2024-01-30T16:40:28Z

includes/class-modal-checkout.php

@@ -54,6 +54,9 @@ public static function init() {
 		add_action( 'woocommerce_checkout_before_customer_details', [ __CLASS__, 'render_before_customer_details' ] );
 		add_filter( 'woocommerce_enable_order_notes_field', [ __CLASS__, 'enable_order_notes_field' ] );
 		add_action( 'woocommerce_checkout_process', [ __CLASS__, 'wcsg_apply_gift_subscription' ] );
+		add_filter( 'woocommerce_order_received_verify_known_shoppers', '__return_false' );
+		// TODO: Remove once we apply auth flow to checkout modal.
+		add_filter( 'newspack_reader_activation_should_render_auth', '__return_false' );


Noting that I've added this with the latest merge of master into epic/ras-acc since we have another task to integrate the auth flow into checkout.

It looks like our Sign Up / Sign In button styles are included in the auth.css file this enqueues -- not sure if just deleting the other files would be better, or maybe something else?

Here's when you're not logged in (getting both the desktop button [with some theme styles] and the unstyled mobile button, which is normally hidden on desktop:

And when you're logged in, you just get the unstyled mobile button:

(I totally could be missing the plan for these specific styles; I'm playing a little catch-up while reviewing this PR 😅 if yes, please disregard!)

It looks like our Sign Up / Sign In button styles are included in the auth.css file this enqueues -- not sure if just deleting the other files would be better, or maybe something else?

Ah! Good catch. I should note that I am only temporarily bypassing the auth flow for this and the other modal checkout ui update PR. But will be incorporating auth into modal checkout with 0/1206274567818686/1205668937699524. Not sure if it's okay to allow these buttons to appear broken in epic/ras-acc for a short time only?

Ah gotcha!

Not sure if it's okay to allow these buttons to appear broken in epic/ras-acc for a short time only?

I think that's probably fine -- it looks like some other stuff will be in flux as well!

chickenn00dle · 2024-01-30T16:50:29Z

Can you please add a list of these breakages to PR description? I've seen this one when testing, not sure if it's expected

Sure thing @adekbadek!

It is expected. This is the checkout form which this PR only changes the modal wrapper for. I've updated the description to show the parts of the checkout flow which are expected not to be fully styled as of this PR.

laurelfulford

Apologies for taking a bit to get back to this, @chickenn00dle! I added some comments, but I may have missed where some of this is being addressed elsewhere! If that's the case, please disregard -- ditto if any of this would be better to handle in separate PRs 🙂

Just let me know if you have any questions at all!

laurelfulford · 2024-01-31T02:32:46Z

includes/class-modal-checkout.php

@@ -54,6 +54,9 @@ public static function init() {
 		add_action( 'woocommerce_checkout_before_customer_details', [ __CLASS__, 'render_before_customer_details' ] );
 		add_filter( 'woocommerce_enable_order_notes_field', [ __CLASS__, 'enable_order_notes_field' ] );
 		add_action( 'woocommerce_checkout_process', [ __CLASS__, 'wcsg_apply_gift_subscription' ] );
+		add_filter( 'woocommerce_order_received_verify_known_shoppers', '__return_false' );
+		// TODO: Remove once we apply auth flow to checkout modal.
+		add_filter( 'newspack_reader_activation_should_render_auth', '__return_false' );


It looks like our Sign Up / Sign In button styles are included in the auth.css file this enqueues -- not sure if just deleting the other files would be better, or maybe something else?

Here's when you're not logged in (getting both the desktop button [with some theme styles] and the unstyled mobile button, which is normally hidden on desktop:

And when you're logged in, you just get the unstyled mobile button:

(I totally could be missing the plan for these specific styles; I'm playing a little catch-up while reviewing this PR 😅 if yes, please disregard!)

laurelfulford · 2024-01-31T02:46:48Z

src/modal-checkout/modal.js

@@ -129,7 +139,9 @@ domReady( () => {
 			form.addEventListener( 'submit', ev => {
 				const formData = new FormData( form );
 				// Clear any open variation modal.
-				variationModals.forEach( variationModal => variationModal.classList.remove( 'open' ) );
+				variationModals.forEach( variationModal => {


The modal works like a charm now, thanks! 🙌

Does this exist already?

Not yet! I made an Asana task for this here, just so we have it tracked somewhere: 1205234045751551-as-1206482127342706

I'm happy to take a swing at it once I'm back to a regular work schedule if that would help!

laurelfulford · 2024-01-31T03:22:06Z

src/modal-checkout/modal.js

 				if ( window.newspackReaderActivation?.overlays ) {
 					modalCheckout.overlayId = window.newspackReaderActivation?.overlays.add();
 				}
 				iframeResizeObserver = new ResizeObserver( entries => {
 					if ( ! entries || ! entries.length ) {
 						return;
 					}
-					const contentRect = entries[ 0 ].contentRect;
-					if ( contentRect ) {
-						modalContent.style.height = contentRect.top + contentRect.bottom + 'px';


This looks good overall!

I did notice the spinner's no longer displaying -- it's using the Newspack UI classes but spinner styles don't exist there.

Similar to this comment, this could be something that just lives in the checkout modal, I think the CSS classes just need to be updated in the markup to not switch to the newspack-ui__ classes, and the CSS itself might need a bit of a massage so the structure still works with how the modal might be structured with the newspack-ui classes 🙂

chickenn00dle · 2024-01-31T15:29:41Z

Thanks for having another look @laurelfulford!

I've commented on your feedback. I'm happy to move styles for things into this PR but wanted to make sure you were aware of all of my updates before making any changes.

laurelfulford

Thanks for all the exhaustive back and forth on this, @chickenn00dle! I think stuff is looking good overall -- thank you for helping me get up to speed where the other pieces are!

I think this piece is good to go, and probably will probably make things easier for the other PRs to have it merged 😅

adekbadek · 2024-02-01T09:23:12Z

Note: #1653 updates the styling of the checkout modal to fix the issues introduced in this PR.

matticbot · 2024-02-06T00:50:04Z

🎉 This PR is included in version 3.0.0-epic-ras-acc.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

matticbot · 2024-02-26T09:03:03Z

🎉 This PR is included in version 3.1.0-epic-ras-acc.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

matticbot · 2024-05-22T16:38:26Z

🎉 This PR is included in version 3.2.0-epic-ras-acc.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

matticbot · 2024-05-29T16:16:28Z

🎉 This PR is included in version 3.3.0-epic-ras-acc.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

chickenn00dle changed the base branch from master to epic/ras-acc January 23, 2024 18:45

chickenn00dle changed the title ~~Feat/update checkout block UI~~ feat: update thank you page with new newspack ui Jan 23, 2024

chickenn00dle force-pushed the feat/update-checkout-block-ui branch 2 times, most recently from 016b4e5 to 0cd1aa7 Compare January 24, 2024 22:19

chickenn00dle commented Jan 25, 2024

View reviewed changes

chickenn00dle mentioned this pull request Jan 25, 2024

feat(newspack-ui): update newspack ui to accomodate modal checkout block Automattic/newspack-plugin#2878

Merged

6 tasks

chickenn00dle force-pushed the feat/update-checkout-block-ui branch from 1e113bb to 12cfec5 Compare January 25, 2024 22:10

chickenn00dle marked this pull request as ready for review January 25, 2024 22:11

chickenn00dle requested a review from a team as a code owner January 25, 2024 22:11

chickenn00dle added the [Status] Needs Review label Jan 25, 2024

chickenn00dle changed the title ~~feat: update thank you page with new newspack ui~~ feat(modal-checkout): update thank you page with new newspack ui Jan 25, 2024

laurelfulford requested changes Jan 26, 2024

View reviewed changes

chickenn00dle force-pushed the feat/update-checkout-block-ui branch from a31242d to 3f03cf3 Compare January 26, 2024 17:38

chickenn00dle requested a review from laurelfulford January 26, 2024 17:45

chickenn00dle commented Jan 26, 2024

View reviewed changes

adekbadek reviewed Jan 30, 2024

View reviewed changes

includes/class-modal-checkout.php Outdated Show resolved Hide resolved

adekbadek reviewed Jan 30, 2024

View reviewed changes

chickenn00dle added 10 commits January 30, 2024 11:16

feat(modal-checkout): add ui to thankyou

7dc6c77

feat(modal-checkout): remove redundant styles

fc01e3c

feat(modal-checkout): update thankyou form classes

7adecee

feat(modal-checkout): fix visibility toggling

84c1db5

feat(modal-checkout): modal fixes

c24716c

feat(modal-checkout): fix iframe height bug

a0e99ac

feat(modal-checkout): apply default styles

9cf857c

feat(modal-checkout): various ui fixes

11da777

feat(modal-checkout): fix variable modal checkout

2b81581

feat(modal-checkout): remove redundant class

82638d4

chickenn00dle added 2 commits January 30, 2024 11:18

feat(modal-checkout): pass class prefix directly

e188f8f

chore(modal-checkout): fix stray merge bugs

1544407

chickenn00dle force-pushed the feat/update-checkout-block-ui branch from e123773 to 1544407 Compare January 30, 2024 16:39

chickenn00dle commented Jan 30, 2024

View reviewed changes

chore(modal-checkout): fix docblock typo

6fa1470

chickenn00dle mentioned this pull request Jan 30, 2024

feat(modal-checkout): update modal checkout checkout form with newspack ui #1653

Merged

6 tasks

laurelfulford requested changes Jan 31, 2024

View reviewed changes

chickenn00dle requested a review from laurelfulford January 31, 2024 15:29

laurelfulford approved these changes Feb 1, 2024

View reviewed changes

github-actions bot added [Status] Approved and removed [Status] Needs Review labels Feb 1, 2024

chickenn00dle merged commit 287d1ef into epic/ras-acc Feb 1, 2024
18 checks passed

chickenn00dle deleted the feat/update-checkout-block-ui branch February 1, 2024 15:25

matticbot added the released on @epic/ras-acc label Feb 6, 2024

	<body class="newspack-ui" id="newspack_modal_checkout">
	<body class="newspack-ui newspack-ui-modal__content" id="newspack_modal_checkout">

feat(modal-checkout): update thank you page with new newspack ui #1649

feat(modal-checkout): update thank you page with new newspack ui #1649

Conversation

chickenn00dle commented Jan 23, 2024 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurelfulford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adekbadek commented Jan 30, 2024

adekbadek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chickenn00dle commented Jan 30, 2024

laurelfulford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chickenn00dle commented Jan 31, 2024

laurelfulford left a comment

Choose a reason for hiding this comment

adekbadek commented Feb 1, 2024

matticbot commented Feb 6, 2024

matticbot commented Feb 26, 2024

matticbot commented May 22, 2024

matticbot commented May 29, 2024

chickenn00dle commented Jan 23, 2024 •

edited

Loading