Add Product Image Gallery #8235
Conversation
Add Product Image Gallery
|
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
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: +861 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
…ocks into add/8233-product-image-gallery-block
|
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
gigitux
removed
the
status: stale
gigitux
force-pushed
the
add/8233-product-image-gallery-block
branch
from
c768772 to
1dc7baa
Compare
gigitux
added
type: new block
woocommercebot
requested review from
a team and
imanish003
and removed request for
a team
….com/woocommerce/woocommerce-blocks into add/8233-product-image-gallery-block
gigitux
force-pushed
the
add/8233-product-image-gallery-block
branch
from
dc64fb3 to
75d00bb
Compare
There was a problem hiding this comment.
Hi @gigitux, I have tested the PR & everything is working as expected after adding product class. Excellent work 🙌🏻 💪🏻
I have left a few minor suggestions, but those shouldn't block this PR. Therefore, I am pre-approving the PR.
assets/js/atomic/blocks/product-elements/product-image-gallery/edit.tsx
Outdated
Show resolved
Hide resolved
assets/js/atomic/blocks/product-elements/product-image-gallery/editor.scss
Outdated
Show resolved
Hide resolved
….com/woocommerce/woocommerce-blocks into add/8233-product-image-gallery-block
|
Thanks for the review and feedback! I addressed your comments. |
|
Before merging the PR, I prefer to wait for feedback from @vivialice. |
gigitux
force-pushed
the
add/8233-product-image-gallery-block
branch
from
bfd2e98 to
dc48ddf
Compare
|
@imanish003 JFYI I removed the save function because it is not necessary for the server-side blocks 👍 e526eff |
| } ) => { | ||
| let currentTemplateId: string | undefined; | ||
|
|
||
| subscribe( () => { |
There was a problem hiding this comment.
Could we use debounce here to make this trigger less frequent?
There was a problem hiding this comment.
Mmm, I would avoid a debounce here to avoid any kind of race condition.
There was a problem hiding this comment.
I'm curious what can be the race condition and its drawback. The motivation behind my suggestion is to keep the editor performant.
There was a problem hiding this comment.
Mmm, for example, the user is editing another template, but the function isn't called due to the debounce and the block is still visible in the inserter.
There was a problem hiding this comment.
but the function isn't called due to the debounce
I don't think we will have this issue, as we just want to debounce them (by 100ms, for example), the subscription still triggers, but less frequently. So when the user switches to another template, the subscription may not trigger right away, but still fast enough to register/unregister the block before user interaction.
We don't have to do it now btw, we can go with the current PR and come back to this later when we see it affect the editor performance.
|
Hey @gigitux ! 👋 For the block icon, I think it's best to use the same I took a look at the preview, and we should align with the greys used in wireframe layouts in core. However, I did notice there are two styles being used to represent images: Screenshot of one of the Wireframe pattern image placeholders: Screenshot of the Image block placeholder: I think it's best to align with the Wireframe style. I have created a mockup:
Do you need an image or svg to implement the above? For the block description, let's use |
|
Thanks @vivialice for the feedback! 🙇
Could you share the SVG? |
…ocks into add/8233-product-image-gallery-block
…ocks into add/8233-product-image-gallery-block
|
Via 8aee841 I updated the package |
|
@vivialice I updated the image, this is the preview:
I'm going to merge the PR, feel free to comment here so that I can create a follow-up PR addressing your feedback! 🙇 |
gigitux
force-pushed
the
add/8233-product-image-gallery-block
branch
from
c9551ab to
e3be328
Compare
This PR adds the Product Image Gallery block. It is possible to add this block only on the Single Product Image Gallery thanks to this.
Design Feedback
@vivialice could give me feedback about:
Notes for testing this PR.
Because of #8314, the styles of this block are not loaded. To avoid this issue, add the class
productto the main div that wraps the entire page.Fixes #8233
Testing
User Facing Testing
Product Image Gallery. Save it.WooCommerce Visibility
Performance Impact
Changelog