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

[Experimental] Utilize WP generated class names to style rating filter block. #44072

Merged
merged 5 commits into from Jan 29, 2024

Conversation

samueljseay
Copy link
Contributor

Changes proposed in this Pull Request:

@dinhtungdu and I agreed that we don't need to create our own unique class names or use bem style syntax, its overly verbose and doesn't add anything.

Instead we scope all styles to the class name generated for the block by WP, and also remove the BEM style prefix from any scoped class names. (To keep PR's small and testable I'm addressing one block at a time.)

Additionally, I noticed a lot of copy pasted styles from the old block that we don't use or need.

This is part of #43167

How to test the changes in this Pull Request:

  1. Much like in [Experimental] Utilize WP generated class names when styling new active filters block. #44069, edit the product catalog or a page and alongside product collection add rating filter.
  2. Also in editor try different modes like dropdown/multiple etc
  3. Check that styling looks good in editor and frontend.

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

[Experimental] Utilize WP generated class names to style rating filter block.

Comment

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Jan 25, 2024
@woocommercebot woocommercebot requested a review from a team January 25, 2024 09:26
Copy link
Contributor

github-actions bot commented Jan 25, 2024

Hi @dinhtungdu,

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

@import "../../../shared/styles/style";

.wc-block-rating-filter {
.wp-block-woocommerce-product-filter-rating {
&.is-loading {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually not convinced we need this but it does target the class on the empty loading state in the editor. I've left it as is for now, but happy to remove it because after removing it I didn't notice anything looking wrong in the styles.

Copy link
Contributor

github-actions bot commented Jan 25, 2024

Test Results Summary

Commit SHA: a05eaf3

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 36s
E2E Tests29100802996m 17s

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.

@samueljseay samueljseay removed the request for review from a team January 25, 2024 10:11
@samueljseay samueljseay reopened this Jan 26, 2024
dinhtungdu
dinhtungdu previously approved these changes Jan 26, 2024
Copy link
Member

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

Wow. 6 added 179 removed. I'm always pleased to review PRs simplifying the codebase. Great work!
Everything still works as expected except the count in checkbox list.

image

@dinhtungdu dinhtungdu self-requested a review January 26, 2024 04:08
@dinhtungdu dinhtungdu dismissed their stale review January 26, 2024 04:09

I was gonna comment only, not approve.

@samueljseay
Copy link
Contributor Author

@dinhtungdu thanks good spotting of the count bug. I've fixed it here

@samueljseay samueljseay requested review from dinhtungdu and removed request for dinhtungdu January 26, 2024 06:50
Copy link
Member

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

Thanks for the update, this is LGTM now.

@samueljseay samueljseay merged commit cfd8f36 into trunk Jan 29, 2024
45 of 46 checks passed
@samueljseay samueljseay deleted the dev/unify-class-naming-rating-filter branch January 29, 2024 06:50
@github-actions github-actions bot added this to the 8.7.0 milestone Jan 29, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Jan 29, 2024
@nigeljamesstevenson nigeljamesstevenson added 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 Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants