This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Product Gallery Thumbnails: Fix overflow issues and improve responsiveness #11665
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
|
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: +952 B (0%) Total Size: 1.61 MB
ℹ️ View Unchanged
|
…and better control the width of the thumbnails
woocommercebot
requested review from
a team and
tjcafferkey
and removed request for
a team
danieldudzic
changed the title
|
So this change breaks the styling when inside the dialog.
Also (not related to this PR). I feel the thumbnails should be a fixed size whether they are right/left/bottom position. In addition, I believe they're currently too big. I've had a look at other popular e-commerce sites, none of them have thumbnails this big. |
You are right, there's some wonkiness with a larger container width. I guess this should be resolved by addressing:
If we hardcode the size there's no way for the user to adjust it. I think it makes sense to adjust the thumbnail size to the container width. Patterns/users can use width controls to control the thumbnail size. The bottom position thumbnail size adjusts to fit all thumbnails in one line in this PR, as we don't have a setting for the thumbnail size. I have tried to find the best thumbnail solution based on the inspiration from this discussion: p1697706500523259-slack-C02FL3X7KR6 - but perhaps there's a different preferred solution when min/max settings are not present. Would you mind sharing your two cents on this, @jarekmorawski? |
danieldudzic
added a commit
that referenced
this pull request
Nov 10, 2023
…idth based on the total number of thumbnails set
…Image and update the width inside of the Dialog
|
@danieldudzic and I had a chat about different ways to solve the sizing issue and we've decided to keep the scaling behavior while tweaking the layout when the thumbnails are on the right or left to always match the large image's size. We've also decided to limit the min. number of thumbnails to 4 or 3 and treat that as a reasonable min. size limit. Here's a simple demo: galleryscale.movWe'll update the issue with any new information and pivot if the new approach doesn't work out. |
…o 11865-update-wordpresse2e-test-utils-playwright-package
…' of https://github.com/woocommerce/woocommerce-blocks into fix/product-gallery-thumbnails-sizing
…b.com/woocommerce/woocommerce-blocks into fix/product-gallery-thumbnails-sizing
danieldudzic
added
the
block: product gallery
…ition of the thumbnails
…b.com/woocommerce/woocommerce-blocks into fix/product-gallery-thumbnails-sizing
gigitux
approved these changes
Nov 24, 2023
There was a problem hiding this comment.
Amazing work! 👏 Everything LGTM. I want to highlight two things that I don't think are related to this PR:
There is a weird animation when the layout of the Product Gallery changes. I think that this is low priority:
https://github.com/woocommerce/woocommerce-blocks/assets/4463174/4f0ef79a-7ad6-468d-9d50-004f8eaf122d
Additionally, I observed that when a product contains only one image, we display the thumbnails:
During my previous involvement with this, there was a specific custom logic in place to address this situation.
Please, if you do not work on fixing the waitForTimeout in this PR, create a follow-up issue.
...locks/product-gallery-thumbnails/product-gallery-thumbnails.block_theme.side_effects.spec.ts
Show resolved
Hide resolved
… at least 2 thumbnails to display
This animation comes from Gutenberg. I suspect that it's a result of the
I have now fixed that. Great catch! |
danieldudzic
added a commit
that referenced
this pull request
Nov 24, 2023
* Product Gallery Thumbnails: Refactor sizing in the editor and the front end * Product Gallery Thumbnails: Change default vertical alignment to top and better control the width of the thumbnails * Product Gallery Thumbnails: Fix thumbnails cropping based on the 'Crop images to fit' setting * Product Gallery Thumbnails: Revert thumbnails styling from #11665 * Product Gallery Thumbnails: Update the default value of the cropImages setting to false
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.
2to3.What
Fixes #11014
Why
Currently, the thumbnails have a hardcoded size and overflow the container.
Important notes
Testing Instructions
Please consider any edge cases this change may have, and also other areas of the product this may impact.
Appearance > Editor.Templates > Single Product.Convert to Blocksif you are using the Classic Templates.Product Galleryblock.Number of Thumbnailsvalues.Positionset tooff,left,bottomandtop.Screenshots or screencast
Editor
Front end
WooCommerce Visibility
Required:
Checklist
Required:
[type]label or a[skip-changelog]label.Conditional:
[skip-changelog]label is not present).Changelog