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

Allow usage of block notice templates when using classic themes #45164

Merged
merged 9 commits into from Mar 14, 2024

Conversation

nielslange
Copy link
Member

@nielslange nielslange commented Feb 27, 2024

Submission Review Guidelines:

Changes proposed in this Pull Request:

Closes #44538.

This PR enables merchants to display the blocks notice templates even when using classic themes. This can be accomplished by overwriting the template files or by using the following filter:

add_filter( 'woocommerce_use_block_notices_in_classic_theme', '__return_true' );

Apart from adjusting the display logic, I've also implemented e2e tests to ensure that this option remains working. In total, we're now having the following e2e tests regarding notice templates:

  • Display default classic notice template when using classic theme.
  • Display custom classic notice template on overwrite when using classic theme.
  • Display default block notice template on filter when using classic theme.
  • Display custom block notice template on overwrite when using classic theme.
  • Display default block notice template when using block theme.
  • Display default block notice template on filter when using block theme.
  • Display custom block notice template on overwrite when using block theme.
  • Display custom classic notice template on overwrite when using block theme.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Install the Storefront theme.
  2. Install and activate the storefront-child__block-notices-template.zip theme.
  3. Create a test page and add the following shortcode to it:
[woocommerce_cart]
  1. Go to /wp-admin/admin.php?page=wc-settings&tab=advanced and set the test page of the previous step as the Cart page.
  2. Go to /wp-admin/edit.php?post_type=shop_coupon and create a coupon.
  3. Go to the frontend and add a product to the cart.
  4. Go to the test page with the cart shortcode.
  5. Provide the coupon code and verify that the block notice success template (green) is visible.
  6. Provide the same coupon code again and verify that the block notice error template (red) is visible.
  7. Delete the product from the cart and verify that the block notice info template (blue) is visible.
  8. Activate the Storefront theme.
  9. Install and activate the Code Snippets plugin.
  10. Add the following code snippet:
add_filter( 'woocommerce_use_block_notices_in_classic_theme', '__return_true' );
  1. Repeat steps 6. until 10. and verify that the block notice templates remain visible.

Apart from the previous testing instructions, also have a look at the added and adjusted e2e tests and verify that all e2e tests are passing.

Screenshots

Block notice success template:

Screenshot 2024-02-27 at 14 05 11
Block notice error template:

Screenshot 2024-02-27 at 14 05 29
Block notice info template:

Screenshot 2024-02-27 at 14 05 43

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Allow usage of block notice templates when using classic themes.

Comment

@nielslange nielslange requested a review from opr February 27, 2024 13:09
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 27, 2024
Copy link
Contributor

github-actions bot commented Feb 27, 2024

Hi @opr, @senadir,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@nielslange nielslange force-pushed the update/44538-allow-block-notices-for-classic-themes branch from ab384e9 to c8b66b0 Compare February 27, 2024 13:34
@nielslange nielslange self-assigned this Feb 27, 2024
@nielslange nielslange closed this Feb 27, 2024
@nielslange nielslange reopened this Feb 27, 2024
Copy link
Contributor

github-actions bot commented Feb 27, 2024

Test Results Summary

Commit SHA: aade6cc

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 38s
E2E Tests309004203516m 23s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

Copy link
Contributor

@opr opr left a comment

Choose a reason for hiding this comment

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

Works OK for me, but left a couple of comments that need answers/addressing before approving. Thanks Niels

@nielslange nielslange requested a review from opr March 5, 2024 10:16
@nielslange
Copy link
Member Author

Thanks for your review, @opr. I've addressed your feedback. Currently, there are two test failing. Nadir firstly mentioned these failing tests in p1709562740110129-slack-C03CPM3UXDJ and Bart mentioned that he'll be looking into them today. As these failing tests seem unrelated to this PR, you could go ahead and re-review the PR again, when you have time.

opr
opr previously requested changes Mar 8, 2024
Copy link
Contributor

@opr opr left a comment

Choose a reason for hiding this comment

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

This looks much better now Niels - there are some checks failing - https://github.com/woocommerce/woocommerce/actions/runs/8200519026/job/22427557137?pr=45164

Please could you address those and get a fresh review from someone else on Rubik as I'll be AFK for a week now.

@opr opr requested review from a team and senadir and removed request for a team March 8, 2024 18:21
@nielslange nielslange requested a review from wavvves March 12, 2024 16:50
Copy link
Contributor

@wavvves wavvves left a comment

Choose a reason for hiding this comment

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

Wow, I bet this started out as something that would be tiny, and grew into a lot of changes and tests 😅

Nice work @nielslange, approving 👍🏼

@nielslange
Copy link
Member Author

Thanks for reviewing this PR, @wavvves. This PR indeed started small and then kept growing and growing. You can probably tell by the amount of test-related files. 😆 That said, thanks again for reviewing it. 🙌

@nielslange nielslange requested a review from opr March 14, 2024 12:18
@nielslange nielslange dismissed opr’s stale review March 14, 2024 12:33

The review feedback had been successfully addressed.

@nielslange nielslange merged commit a738357 into trunk Mar 14, 2024
65 checks passed
@nielslange nielslange deleted the update/44538-allow-block-notices-for-classic-themes branch March 14, 2024 12:33
@github-actions github-actions bot added this to the 8.8.0 milestone Mar 14, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Mar 14, 2024
Copy link
Member

@senadir senadir left a comment

Choose a reason for hiding this comment

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

FWIW I just ended up testing this and things works fine! great work with the tests Niels.

@veljkho veljkho added needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Mar 14, 2024
@nigeljamesstevenson nigeljamesstevenson added the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Mar 25, 2024
@PaulSchiretz
Copy link

PaulSchiretz commented Apr 20, 2024

Very nice fix! @nielslange,

Just one thing, if i do NOT opt in to show block notices in a classic theme wp_enqueue_style( 'wc-blocks-style' ); is still called on wp_head and there is no way to dequeue it.

	/**
	 * Initialize notice hooks.
	 */
	public function init() {
		add_action(
			'after_setup_theme',
			function() {
				/**
				 * Allow classic theme developers to opt-in to using block notices.
				 *
				 * @since 8.8.0
				 * @param bool $use_block_notices_in_classic_theme Whether to use block notices in classic theme.
				 * @return bool
				 */
				if ( wp_is_block_theme() || apply_filters( 'woocommerce_use_block_notices_in_classic_theme', false ) ) {
					add_filter( 'wc_get_template', [ $this, 'get_notices_template' ], 10, 5 );
				}
			}
		);

		add_filter( 'woocommerce_kses_notice_allowed_tags', [ $this, 'add_kses_notice_allowed_tags' ] );
		add_action( 'wp_head', [ $this, 'enqueue_notice_styles' ] );
	}

To work around this it wouldn't it make sense to also check the conditionals in enqueue_notice_styles function?
The change would be very minor but would allow us to get rid of some unused css.

Simply replace:

	/**
	 * Replaces all notices with the new block based notices.
	 *
	 * @return void
	 */
	public function enqueue_notice_styles() {
		wp_enqueue_style( 'wc-blocks-style' );
	}

with:

	/**
	 * Replaces all notices with the new block based notices.
	 *
	 * @return void
	 */
	public function enqueue_notice_styles() {
		if ( wp_is_block_theme() || apply_filters( 'woocommerce_use_block_notices_in_classic_theme', false ) ) {
			wp_enqueue_style( 'wc-blocks-style' );
		}
	}

I know I'm late to the party, just hope someone reads this ;-)

@nielslange
Copy link
Member Author

nielslange commented Apr 22, 2024

Thank you for reaching out, @PaulSchiretz. Initially, I placed the call add_action( 'wp_head', [ $this, 'enqueue_notice_styles' ] ); outside the conditional logic to accommodate merchants using classic themes who might be overriding the notice templates and wish to use the block notice templates. Subsequently, we introduced the woocommerce_use_block_notices_in_classic_theme filter, which indeed makes it feasible to incorporate add_action( 'wp_head', [ $this, 'enqueue_notice_styles' ] ); within the conditional logic.

Instead of:

/**
 * Replaces all notices with the new block-based notices.
 *
 * @return void
 */
public function enqueue_notice_styles() {
	if ( wp_is_block_theme() || apply_filters( 'woocommerce_use_block_notices_in_classic_theme', false ) ) {
		wp_enqueue_style( 'wc-blocks-style' );
	}
}

We could consider updating our approach to embed the action into the conditional statement:

/**
 * Initialize notice hooks.
 */
public function init() {
	add_action(
		'after_setup_theme',
		function () {
			/**
			 * Allow classic theme developers to opt-in to using block notices.
			 *
			 * @since 8.8.0
			 * @param bool $use_block_notices_in_classic_theme Whether to use block notices in classic themes.
			 * @return bool
			 */
			if ( wp_is_block_theme() || apply_filters( 'woocommerce_use_block_notices_in_classic_theme', false ) ) {
				add_filter( 'wc_get_template', [ $this, 'get_notices_template' ], 10, 5 );
				add_action( 'wp_head', [ $this, 'enqueue_notice_styles' ] );
			}
		}
	);

	add_filter( 'woocommerce_kses_notice_allowed_tags', [ $this, 'add_kses_notice_allowed_tags' ] );
}

However, this change would require all merchants who are customizing classic notice templates to use block notice templates to add the following call to their sites, ensuring that the necessary styles are loaded:

add_filter( 'woocommerce_use_block_notices_in_classic_theme', '__return_true' );

Considering the styles file is only 2.7kb, we decided against making this change to avoid the potential for disruption. Modifying this logic now could compel all merchants to implement the filter we discussed, which, while straightforward, might lead to temporary disruptions in the display of notices if overlooked, potentially causing frustration.

Let me know if this makes sense to you, or if there are any other concerns we should address.

@PaulSchiretz
Copy link

@nielslange I see, makes total sense. Thanks for clarifiying this to me! I kind of went the other route, once the block templates where the default i tweaked my theme to incooperate the block templates then when the change was reversed i tweaked the classic templates to look like the block templates. Now that this is supported via hook i was wondering if i missed some spot once i saw the additional css file. Eather way as you also said, better to leave the css in than cause another confusion :-) Thanks for the quick clarification!

@nielslange
Copy link
Member Author

Thanks for the quick clarification!

Thank you for reaching out to clarify this, @PaulSchiretz. As for overwriting the notice templates, I posted Tutorial: Overriding notice templates a few weeks ago. You can either use the hook or manually overwrite the notice template files.

That said, thanks again for reaching out, and please don't't hesitate to contact us if you have more questions or suggestions. 🙌

@tiago-123
Copy link

add_filter( 'woocommerce_use_block_notices_in_classic_theme', '__return_true' );

It works like a charm! Thanks @nielslange!

@nielslange
Copy link
Member Author

It works like a charm! Thanks @nielslange!

Yeah! You're very welcome, @tiago-123. 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. plugin: woocommerce Issues related to the WooCommerce Core plugin. release: highlight Issues that have a high user impact and need to be discussed/paid attention to. status: analysis complete Indicates if a PR has been analysed by Solaris team: Rubik
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Allow the use of block notices when using classic themes
8 participants