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

Update Woocommerce templates #1998

Merged
merged 2 commits into from Dec 7, 2022
Merged

Conversation

IanDelMar
Copy link
Contributor

@IanDelMar IanDelMar commented Nov 2, 2022

Description

This PR updates the version number in Woocommerce templates.

Motivation and Context

WooCommerce 7.0.1 adds wc_wp_theme_get_element_class_name() which is a wrapper for wp_theme_get_element_class_name() which calls WP_Theme_JSON::get_element_class_name(). Understrap does not come with a theme.json. Therefor a call to WP_Theme_JSON::get_element_class_name( $element ) gives an empty string and adding wc_wp_theme_get_element_class_name() to the templates can be skipped (would be a breaking change).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Related Issues or Roadmap requests

#1997

@bacoords bacoords added this to the Release 1.3.0 milestone Nov 5, 2022
@bacoords bacoords modified the milestones: Release 1.3.0, Release 1.2.2 Nov 17, 2022
@bacoords
Copy link
Member

@IanDelMar I'd like to fast-track this to do another minor release. I just need to spend some time testing. Let me know what you think.

@IanDelMar
Copy link
Contributor Author

IanDelMar commented Nov 17, 2022

Think about what? Releasing 1.3.0 soon? Actually this doesn't qualify for a feature release, does it? There is no code change at all. It is a patch (1.2.2) at max, no?
What do you think about adding a notice to the readme which versions of WooCommerce are supported?

@bacoords
Copy link
Member

@IanDelMar Exactly! - I already updated the tag to 1.2.2 earlier

And yeah the readme makes sense

@IanDelMar
Copy link
Contributor Author

Oh, didn't notice the updated version! And yes, I think Understrap+WooCommerce users will be happy if you release 1.2.2 soon.

Concerning the supported version: the maximum version is clear. Finding the minimum version is a little cumbersome. I tend to simply making a cut and say v6+ is supported (currently . I'm not sure what that means for the type of change. Is it a breaking change which I guess?

Active installs of WooCommerce as of today: versions from 6.8 account for 45.9% of installs.

image
Source

@IanDelMar
Copy link
Contributor Author

For 1.2.2: We should switch back to inc/editor-color-palette.json with Bootstrap 4 colors.

@bacoords
Copy link
Member

bacoords commented Dec 5, 2022

@IanDelMar Looks like we're just missing the 7.1.0 update to global/form-login.php. From what I can gather, they're adding the woocommerce-Input class to the password field. It's unclear if it's a typo and they meant 7.0.1, but either way the class is missing.

Re: wc_wp_theme_get_element_class_name() - I think we should still include it. I often add a theme.json to my child themes, even if we couldn't really add one to the parent Understrap theme without breaking a million sites. It seems better to include the function for users who might have a theme.json, than just not include it. https://github.com/woocommerce/woocommerce/commits/trunk/plugins/woocommerce/templates/global/form-login.php

@IanDelMar
Copy link
Contributor Author

IanDelMar commented Dec 6, 2022

@bacoords then we have to provide both wc_wp_theme_get_element_class_name() and wp_theme_get_element_class_name()

Simply adding wc_wp_theme_get_element_class_name() would break sites on WooCommerce v6 or WP < 6.1.0.

EDIT: Rechecked wc_wp_theme_get_element_class_name() which checks for the existence of wp_theme_get_element_class_name(). So we have to provide wc_wp_theme_get_element_class_name() only.

`woocommerce-Input` has been added in WC 7.0.0
@IanDelMar
Copy link
Contributor Author

Let's take cart/cart-empty.php as an exmaple:

<a class="btn btn-outline-primary" href="<?php echo esc_url( apply_filters( 'woocommerce_return_to_shop_redirect', wc_get_page_permalink( 'shop' ) ) ); ?>">

We can either add it like so:

<?php echo esc_attr( wc_wp_theme_get_element_class_name( 'button' ) ? ' ' . wc_wp_theme_get_element_class_name( 'button' ) : 'btn btn-outline-primary' ); ?>

or like so

btn btn-outline-primary<?php echo esc_attr( wc_wp_theme_get_element_class_name( 'button' ) ? ' ' . wc_wp_theme_get_element_class_name( 'button' ) : '' ); ?>

The first version might be a breaking change for child themes with a theme.json that defines classes for the button element. The second version does not guarantee that classes added via wc_wp_theme_get_element_class_name() will override btn btn btn-outline-primary.

@bacoords
Copy link
Member

bacoords commented Dec 7, 2022

Ok - we can return to wc_wp_theme_get_element_class_name for 1.3 if we feel like we need it. For now it seems like it's not necessary. I'm going to merge and set up a RC for 1.2.2

@bacoords bacoords merged commit cd847bc into understrap:develop Dec 7, 2022
@IanDelMar IanDelMar deleted the woo-701 branch December 8, 2022 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants