This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Fix Attribute terms sometimes being assigned children #8720
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Attribute terms (e.g. “Blue”, “Yellow”) have their own `id` property which sometimes can overlap with their Taxonomy `id` (e.g. in the test case, “Blue” had the same `id` as the “Size” taxonomy), and the function building the tree representation for the `SearchListControl` would erroneously assign children to it. To fix this, we do an ugly patch here: we rename the `id` key of `AttributeTerm` when changed to a `SearchListItem` to `termId` instead, so we can differentiate. We do this because other items passed to `SearchListControl` might possibly have several layers of children, but we are sure that in the case of terms, we only have on level deep. This may need a better refactoring at some point.
sunyatasattva
added
type: bug
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
assets/js/blocks/attribute-filter/edit.tsx
assets/js/editor-components/search-list-control/search-list-control.tsx |
|
Size Change: +484 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
danielwrobert
added
status: needs review
and removed
status: stale
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
|
I'm converting this PR to a draft as there are side effects of this fix that are not resolved yet. |
github-actions
bot
removed
the
status: stale
|
@kmanijak I'd say we should specify what these side-effects are, so that they can be resolved. |
|
@kmanijak News on this? |
Hey @sunyatasattva, I agree with that!
I was AFK recently, I'll try to revisit that in the upcoming days as I don't remember the exact details from top of my head 🙌 |
|
Here's a video showing the problems of choosing the Product Attributes in the Products block: products-8720.mov |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attribute terms (e.g. “Blue”, “Yellow”) have their own
idproperty which sometimes can overlap with their Taxonomyid(e.g. in the test case, “Blue” had the sameidas the “Size” taxonomy), and the function building the tree representation for theSearchListControlwould erroneously assign children to it.To fix this, we do an ugly patch here: we rename the
idkey ofAttributeTermwhen changed to aSearchListItemtotermIdinstead, so we can differentiate. We do this because other items passed toSearchListControlmight possibly have several layers of children, but we are sure that in the case of terms, we only have on level deep.This may need a better refactoring at some point.
Fixes woocommerce/woocommerce#42438
Testing
User Facing Testing
WooCommerce Visibility