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

Add product archive header template and re-hook #33681

Merged
merged 5 commits into from Mar 8, 2024

Conversation

tommyferry
Copy link
Contributor

@tommyferry tommyferry commented Jun 30, 2022

Moves the product taxonomy archive header into its own template file, creates a related output function, and then re-hooks the new template back into the archive-product.php file in the same place, via the new woocommerce_shop_loop_header hook.

This is so that this template part can be removed/unhooked via the remove_action function, which is a cleaner approach than template overrides or additional style changes. Moreover, this is consistent with the rest of the archive-product.php template, which is already being rendered via actions.

All Submissions:

Changes proposed in this Pull Request:

Closes #33672

How to test the changes in this Pull Request:

  1. View the shop archive or a product category archive with this patch applied.
    Result: Archive header should appear as normal.
  2. Add the following to the test theme functions.php file (or via another route):
    remove_action( 'woocommerce_shop_loop_header', 'woocommerce_product_taxonomy_archive_header', 40 );
    Result: Archive header should be removed from the markup.

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 run tests with your changes locally?
  • Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Jun 30, 2022
@samueljseay samueljseay added needs: developer feedback Issues that need feedback from one of the WooCommerce Core developers. focus: classic front end Issues related to the classic front end. labels May 3, 2023
@samueljseay samueljseay requested review from Konamiman and removed request for Konamiman May 3, 2023 11:12
@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2023

Hi ,

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

@Konamiman
Copy link
Contributor

Hi @tommyferry, sorry for the big delay, this pull request fell through the cracks and I missed it. I have reviewed and tested it and it looks fine, but before merging it it should get a changelog file, I tried to add it myself but the trunk branch of your repository is protected, so could you please either unprotect trunk or add the changelog file yourself? Yo can do that by running pnpm --filter=woocommerce changelog add from tithin the plugins/woocommerce directory. Thank you!

@tommyferry
Copy link
Contributor Author

Hi @Konamiman, thanks for reviewing and pushing this forward. I've followed your suggestion and added the changelog file as well as unprotecting the trunk branch in case you'd like to make any further adjustments. Let me know if there's anything else you need from me on this!

@Konamiman
Copy link
Contributor

@tommyferry Now I see that there are several CI checks that are failing. This might be because the pull request is quite old, could you please do a merge or a rebase from the current trunk?

@tommyferry
Copy link
Contributor Author

Hi @Konamiman, thanks for patience and guidance

I've now merged the current woocommerce trunk into this branch, which should hopefully enable the CI checks to succeed so this can be merged.

@Konamiman
Copy link
Contributor

Hi @tommyferry, sorry but it looks like a new rebase will be needed. I'll be monitoring this more closely this time to try and merge this asap to prevent further CI problems.

Moves the product taxonomy archive header into its own template file,
creates a related output function, and then re-hooks the new template
back into the archive-product.php file in the same place, via the
'woocommerce_before_main_content' hook, priority 40.

This is so that this template part can be removed/unhooked via the
'remove_action' function, which is a cleaner approach than template
overrides or additional style changes.

Resolves: woocommerce#33672
@tommyferry
Copy link
Contributor Author

No worries @Konamiman, I've just rebased so hopefully this should now be correctly passing all tests. Let me know if there's anything else needed on this - it all looks correct to me (and a fairly minor addition) but happy to make further changes as needed. Thanks!

@tommyferry
Copy link
Contributor Author

@Konamiman - I noticed the recent updates were failing due to a missing template version bump. I've added those now

@Konamiman
Copy link
Contributor

@tommyferry Looks like there are still missing template version bumps:

Error: Templates have changed but template versions were not bumped:

### Template changes:
* **File:** /plugins/woocommerce/templates/archive-product.php
  * WARNING: This template may require a version bump!
* **File:** /plugins/woocommerce/templates/loop/header.php
  * WARNING: This template may require a version bump!

@tommyferry
Copy link
Contributor Author

Hey @Konamiman, I've just noticed that too - it's very strange as I bumped one of those and the other is a new file... Perhaps I'm missing something here though, should the version be bumped to the version of WC?

SCR-20240104-njcc-2

@Konamiman
Copy link
Contributor

Hi @tommyferry, indeed the version number expected here is the WooCommerce version where the bump happened, which would be 8.6.0 in this case. So could you please make the change?

@tommyferry
Copy link
Contributor Author

Ahh that makes sense, thanks so much @Konamiman - all updated now. Fingers crossed for the workflows to pass this time :)

@Konamiman
Copy link
Contributor

Hi @tommyferry. I've checked with my team as I wasn't sure why tests were still failing, and turns out that there's a problem with the code, specifically here:

if ( ! function_exists( 'woocommerce_product_taxonomy_archive_header' ) ) {
	function woocommerce_product_taxonomy_archive_header() {
		if ( is_product_taxonomy() ) {
			wc_get_template( 'loop/header.php' );
		}
	}
}

The problem is the if ( is_product_taxonomy() ) line, which wasn't present in the code that the function is replacing. However I understand that the line is needed, since the woocommerce_before_main_content hook is used also in the single product page.

I think that the cleanest solution, also consistent with the rest of the code in archive-product.php, would be adding a new action that would run immediately after the do_action( 'woocommerce_before_main_content' ); line. The name of the action could be woocommerce_shop_loop_header, again for consistency with the other actions in the file. Please keep in mind that we require all new hooks to include a documentation comment block (example).

@tommyferry
Copy link
Contributor Author

Hey @Konamiman, thanks for your help in diagnosing the problem - I've implemented the suggested change now and pushed the update.

I did wonder whether it would be possible to achieve the same thing with an additional is_shop() check but I expect that there would be some other templates or situations that would still be missed so I think this is the best option as well.

@Konamiman Konamiman removed the needs: developer feedback Issues that need feedback from one of the WooCommerce Core developers. label Jan 11, 2024
@jonathansadowski jonathansadowski merged commit 7fe00de into woocommerce:trunk Mar 8, 2024
96 of 101 checks passed
@github-actions github-actions bot added this to the 8.8.0 milestone Mar 8, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Mar 8, 2024
@alopezari alopezari added needs: internal testing Indicates if the PR requires further testing conducted by Solaris 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 11, 2024
Konamiman pushed a commit that referenced this pull request Mar 13, 2024
* Add product archive header template and re-hook

Moves the product taxonomy archive header into its own template file,
creates a related output function, and then re-hooks the new template
back into the archive-product.php file in the same place, via the
'woocommerce_before_main_content' hook, priority 40.

This is so that this template part can be removed/unhooked via the
'remove_action' function, which is a cleaner approach than template
overrides or additional style changes.

Resolves: #33672

* add product archive header template changelog

* bump template versions

* bump template versions correctly

* add a new hook in the archive-product.php template for the header template
@JimmyAppelt
Copy link
Contributor

JimmyAppelt commented Mar 26, 2024

@Konamiman @tommyferry hi here :) seems the template version and function addition version at this time should be 8.8.0 instead of 8.6.0, since it ships with 8.8.0.beta-1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: classic front end Issues related to the classic front end. needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris type: community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Move product taxonomy archive header HTML into its own template and re-hook
7 participants