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] Fix a bug in new attribute filter where we didn't set the attribute from content panel #44757

Merged
merged 4 commits into from Feb 21, 2024

Conversation

samueljseay
Copy link
Contributor

@samueljseay samueljseay commented Feb 19, 2024

Changes proposed in this Pull Request:

Solves #44756

When implementing the attribute filter block, we missed that it was not setting the attribute when editing it from the inspector "content" panel. This fixes that bug.

How to test the changes in this Pull Request:

  1. Add the new product filter: attribute (beta) filter block to a page or template
  2. Choose an attribute to filter by and click done.
  3. Select the block, now open inspector panel, go to "content", try changing the attribute to see it update, also confirm search filtering of the attributes works as before.

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] Fix a bug in new attribute filter where we didn't set the attribute from content panel

Comment

@woocommercebot woocommercebot requested a review from a team February 19, 2024 07:47
Copy link
Contributor

github-actions bot commented Feb 19, 2024

Hi @dinhtungdu, @woocommerce/woo-fse

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

Copy link
Contributor

github-actions bot commented Feb 19, 2024

Test Results Summary

Commit SHA: f6d9db9

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

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 February 19, 2024 11:01
@samueljseay samueljseay reopened this Feb 19, 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.

Code changes look good to me. Can you also fix other issues listed in #44756 in this PR as they're all related to the Content Settings panel?

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 19, 2024
@samueljseay
Copy link
Contributor Author

@dinhtungdu I've addressed the icon styling issue. (Note it was present in the old filter too, so this fixes it for both).

There was no real exact prior art for where it should be positioned so I've just centered everything:

Before:
Screenshot 2024-02-19 at 9 27 23 PM

After:

Screenshot 2024-02-19 at 9 49 32 PM

As for these two:

The Content Settings may not a good label for this panel.
We may not need a separate panel for the attribute selector.

I've left some questions on the issue about this.

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.

I can confirm the styling issue is fixed and attribute can be set from the Content Settings panel now. Thanks for working on this!

@samueljseay samueljseay merged commit ccb1fb3 into trunk Feb 21, 2024
55 of 56 checks passed
@samueljseay samueljseay deleted the dev/fe-filters-fix-content-panel-bug branch February 21, 2024 06:53
@github-actions github-actions bot added this to the 8.7.0 milestone Feb 21, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 21, 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 Feb 21, 2024
dinhtungdu added a commit that referenced this pull request Mar 4, 2024
dinhtungdu added a commit that referenced this pull request Mar 5, 2024
…nspector settings of the new attribute filter block (#45276)

Fix: regression introduced in #44757 that breaks the inspector settings of the new attribute filter block
Konamiman pushed a commit that referenced this pull request Mar 13, 2024
…nspector settings of the new attribute filter block (#45276)

Fix: regression introduced in #44757 that breaks the inspector settings of the new attribute filter block
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