This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Add Product Query Support for Atomic Rating Block #7352
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
danielwrobert
added
type: refactor
|
The release ZIP for this PR is accessible via: |
|
Size Change: +6.82 kB (+1%) Total Size: 999 kB
ℹ️ View Unchanged
|
danielwrobert
force-pushed
the
add/atomic-rating-pq-support
branch
from
00e05be to
58b7473
Compare
rubikuserbot
requested review from
a team and
sunyatasattva
and removed request for
a team
danielwrobert
requested review from
gigitux and
sunyatasattva
and removed request for
sunyatasattva and
gigitux
danielwrobert
force-pushed
the
add/atomic-rating-pq-support
branch
3 times, most recently
from
2de7f19 to
62841a3
Compare
|
Actually, I've changed my mind. I'm going to convert now because I will add a types interface for the context that we'll be able to use on the rest of the blocks. Going forward, however, I think it will be best to approach the TS conversion in separate PRs due to the potential "rabbit-hole" of necessary adjustments to other parts of the code base. |
danielwrobert
force-pushed
the
add/atomic-rating-pq-support
branch
from
62841a3 to
d782eca
Compare
|
Hello @danielwrobert! Thanks for the work you're making here. I'm failing to properly test this PR, I'm not sure what I'm doing wrong.
Is there something obvious that I might be missing? FWIW I feel the test instructions should also include adding the block via the inserter. Also:
Seem to refer to the Add to Cart block. Is there a different behavior for this block as well? |
This is a workaround until we can confirm the correct typ...This is a workaround until we can confirm the correct type from ProductResponseItem. 🚀 This comment was generated by the automations bot based on a
|
Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the |
sunyatasattva
added
the
block-type: product-query
danielwrobert
commented
Oct 14, 2022
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import Block from './block'; | ||
| import withProductSelector from '../shared/with-product-selector'; | ||
| import { BLOCK_TITLE, BLOCK_ICON } from './constants'; | ||
| import { Attributes } from './types'; | ||
| import { ProductQueryContext as Context } from '../../../../blocks/product-query/types'; |
There was a problem hiding this comment.
Just noting that I realize this is not an ideal import approach by using the relative path. However, moving this to an alias includes updating/cleaning up the tsconfig(s) and webpack config - it would also be good to test any potential impact that might raise.
In the interest of keeping this PR scoped to supporting the PQ block, I propose addressing this in a follow-up item.
There was a problem hiding this comment.
What potential impacts do you see here? I agree with you on keeping the PR scoped, but also to not unnecessarily add any other small tasks which could be solved with little effort.
I see that perhaps you mean that if we introduce a @woocommerce/blocks alias path and migrate all our paths it could cause some unintended consequences, is that correct? While I'm not seeing too many unintended consequences for that either, we can add the alias and use it only here (and from now on) to keep this PR correctly scoped.
It's, in a sense, the same principle as not necessarily removing all our TS errors, but not add any further TS error. So here, we don't have to fix all relative paths, just not add wood to the fire, so to speak. And then, like you say, make another refactoring issue and PR to fix the older relative paths (or fix them as they come into play).
So, these are my 2c. But I'm approving the PR because it's such a small thing and if you disagree with it, I'm also ok with that.
sunyatasattva
approved these changes
Oct 27, 2022
danielwrobert
force-pushed
the
add/atomic-rating-pq-support
branch
from
9eab7cb to
b734e8b
Compare
Set up the block for PQ support and add necessary adjustments for the editor. Will address dynamic save functionality in a following commit.
As a workaround, added a general Record type but left a TODO to revisit the proper object, as there is a mismatch in the shape of the default object property types and the actual types.
Allows for the ability to add the rating block from in the inserter (as long as it's an inner block of the listed parents in the config). Also disables the placeholder product selector from being rendered unnecessarily (i.e., when the context ID is present).
Reassign parent array to ancestor array which allows for blocks to be included with more flexibility - i.e., added within groups that are children of the ancestor block.
Some adjustments to utilize types that we already have available, along with some syntax adjustments and more sensible import tweaks.
Instead of using the generic Record, we can utilize the ProductResponseItem interface and set an omission for the average_rating property until that is corrected to properly reflect the API response.
Allows us to use exports from the blocks dir as "external" imports. This way we do not need to write long, relative import paths (which can be fragile in the long run).
danielwrobert
force-pushed
the
add/atomic-rating-pq-support
branch
from
a6fb26c to
72e87b3
Compare
1 task
4 tasks
senadir
pushed a commit
to senadir/woocommerce-blocks
that referenced
this pull request
Nov 12, 2022
* Add PQ support for client-side. Set up the block for PQ support and add necessary adjustments for the editor. Will address dynamic save functionality in a following commit. * Add dynamic render function for PQ support. * Add dynamic render callback for SSR. * Remove client-side Save function. * Add PQ Context interface to shared type defs. * Convert all block JS files to TS. * Remove commented import from block file. * Add typecasting to block function params. As a workaround, added a general Record type but left a TODO to revisit the proper object, as there is a mismatch in the shape of the default object property types and the actual types. * Update inserter behavior. Allows for the ability to add the rating block from in the inserter (as long as it's an inner block of the listed parents in the config). Also disables the placeholder product selector from being rendered unnecessarily (i.e., when the context ID is present). * Update parent inner blocks config. Reassign parent array to ancestor array which allows for blocks to be included with more flexibility - i.e., added within groups that are children of the ancestor block. * Add productID to rating Attributes interface. * TS type casting and import adustments. Some adjustments to utilize types that we already have available, along with some syntax adjustments and more sensible import tweaks. * Update type-casting to use ProductResponseItem Instead of using the generic Record, we can utilize the ProductResponseItem interface and set an omission for the average_rating property until that is corrected to properly reflect the API response. * Add alias to blocks dir for imports. Allows us to use exports from the blocks dir as "external" imports. This way we do not need to write long, relative import paths (which can be fragile in the long run).
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 Rating block.
On the client side, when the Rating Block is used within the Product Query block, the markup will be rendered on the server side - no javascript related to the Rating block will be loaded.
Fixes #7330
Testing
npm start(need to run thestartcommand to catch the experimental flags).woocommerce/product-ratingblock to the Product QueryINNER_BLOCKS_TEMPLATEinassets/js/blocks/product-query/constants.tsunder line 54 (only for testing, remove afterwards).product-rating-frontend.js).product-rating-frontend.jsis loaded.WooCommerce Visibility