woocommerce / storefront

Official theme for WooCommerce
https://wordpress.org/themes/storefront/
972 stars 471 forks source link

Remove styling from No Products filter results. #2027

Closed danielwrobert closed 2 years ago

danielwrobert commented 2 years ago

This is a small styling update that omits the .woocommerce-info styles on the filter results notification when no products match the filter.

See pdnLyh-1cy-p2 for additional context.

Fixes #2026.

Screenshots

Before After
CleanShot 2022-08-11 at 15 07 47 CleanShot 2022-08-11 at 15 05 09

How to test the changes in this Pull Request:

  1. With the Storefront theme active, go to your site's Store page (displaying the WooCommerce Product Catalog) and add some filter blocks (e.g., Filter Products by Price, Filter Products by Attribute, etc.). You can add this directly on the page or in a widget area.
  2. View the frontend and set your filters to where no products will match and be displayed.
  3. Inspect the No products banner to see if it has a woocommerce-no-products-found class. If it does not, add that class via the inspector to see the changes in this PR take effect (the output for the banner comes from this notice in WC Core - see screenshot below).
  4. Confirm the No products message appears as it does in the "After" screenshot above (with no blue banner styling).
  5. Confirm no related JS or PHP errors.

CleanShot 2022-08-17 at 16 02 48

Changelog

Enhancement - Remove styling from No Products filter results. #2026

tjcafferkey commented 2 years ago

If you go onto a product category page, and then apply some of the filters to activate the banner then the blue background is present here (/product-category/clothing/accessories/?filter_color=yellow&query_type_color=or&max_price=13)

I assume this is also the case on product tag pages too.

Screenshot 2022-08-16 at 09 17 19

danielwrobert commented 2 years ago

@tjcafferkey thanks for catching this! In 377f957, I've adjusted the selector to omit any of the archive pages that fall under the store (i.e., the Product post type).

danieldudzic commented 2 years ago

Tested, and it works fine for me both on the shop and archive pages. Code also looks fine.

Aljullu commented 2 years ago

I'm a bit concerned that this change might affect other notices besides the "No products were found..." one. I can't think of any examples, but there might be cases where WC or an extension add an info notice that needs to be visible in those pages but is not the one we want to remove the style from.

I wonder if we should opt for an alternative approach:

Thoughts? :thinking:

danielwrobert commented 2 years ago

@Aljullu you raise a good point, perhaps this has the potential to be a bit too broad.

I wouldn't recommend removing the class from that WC Core file because that would have an impact on all themes that rely on it, not just Storefront.

I do like the idea of adding a class there though (something like woocommerce-no-products-found or woocommerce-no-products). That way we can better ensure we're targeting the banner in the specific context that we expect.

I'll go ahead and create a PR and adjust this approach when that gets merged. Thanks!

Note: converting back to Draft and setting status as blocked until the aforementioned adjustment is made in WC Core.

danielwrobert commented 2 years ago

I created a small PR here to add a contextual class for the output.

I've adjusted the styling changes in 5078618 so that the changes will take effect when the WC Core PR is merged.

Since the changes are all in CSS and non-breaking, they shouldn't depend on one-another as far as merging goes.

I've also updated the test plan above.

tjcafferkey commented 2 years ago

This is a better approach I think @danielwrobert, only thing I have noticed is that what I am seeing does not look like the screenshot you've posted in "After" because there is still padding around the notice which makes it not align with the title and look a bit strange. I think its due to the padding added in this commit https://github.com/woocommerce/storefront/pull/2027/commits/5078618b70c6c877316a729a2dc0a46822353607

Screenshot 2022-08-22 at 11 22 01

danielwrobert commented 2 years ago

@tjcafferkey good eye, thanks for spotting that! I meant to zero out the side padding there. I've updated it in 93ad4f87.

danielwrobert commented 2 years ago

Noting for posterity: @tjcafferkey as discussed in Slack the failing PHP test is unrelated to this PR and seems to be due to a bunch of missing docblock comments.

There is an open Issue (#2004) to investigate this.