This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Add product query support for Product Summary block #7774
Merged
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
On the client side, when the Product Summary block is used within the product query block, the markup will be rendered on the server side - No javascript related to Product Summary block will be rendered.
imanish003
added
skip-changelog
|
The release ZIP for this PR is accessible via: |
TypeScript Errors ReportFiles with errors: 432
assets/js/atomic/blocks/product-elements/summary/attributes.ts
assets/js/atomic/blocks/product-elements/summary/index.ts |
|
Size Change: -3 B (0%) Total Size: 972 kB
ℹ️ View Unchanged
|
rubikuserbot
requested review from
a team and
Aljullu
and removed request for
a team
|
Curious here: why skip changelog? I think this is changelog-worthy. |
Aljullu
reviewed
Nov 29, 2022
There was a problem hiding this comment.
Thanks for creating this PR, @imanish003! This is testing well and code overall looks good. However, I think we should escape all values we print in render().
Even when they come from a source we know (ie: StyleAttributesUtils::get_classes_and_styles_by_attributes()), we should be escaping everything we print in the HTML. Otherwise, we might introduce a filter in get_classes_and_styles_by_attributes() in the future, and a plugin would be able to modify the output and print some malicious code in there.
I saw #7709, #7675 and #7734 are also affected by this. Will you be able to update them (if the PR is open) or create a new PR fixing it (if the original PR has already been merged?). We would need this to be fixed before the release next Monday.
|
Hi @Aljullu, |
Aljullu
reviewed
Nov 30, 2022
There was a problem hiding this comment.
Thanks for updating this PR, @imanish003! I had a small issue when the block had some custom styling, but besides that, everything looks good. I commented it inline below.
@sunyatasattva I guess it's because the Product Query block hasn't been released yet (until next release), so probably there is no need to mention all its improvements in the changelog until it has been published, no? |
Yep, @Aljullu, we talked privately with Manish and decided that from next release onwards, these kind of changes should have changelog entries, but for now it's ok like this. The downside is that, as we release, these won't appear on the list. But I guess our Release Post can cover everything. |
More info: #7774 (comment)
Aljullu
approved these changes
Dec 1, 2022
There was a problem hiding this comment.
This is testing well and code looks good. Thanks @imanish003! 🚢
imanish003
added a commit
that referenced
this pull request
Dec 2, 2022
This reverts commit ff0b05e.
imanish003
added a commit
that referenced
this pull request
Dec 2, 2022
7 tasks
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.
Adds Product Query support for the atomic Product Summary block
On the client side, when the Product Summary block is used within the product query block, the markup will be rendered on the server side - No javascript related to Product Summary block will be rendered.
Fixes #7334
Testing
Screen.Recording.2022-11-29.at.12.01.41.PM.mov
WooCommerce Visibility