Add media sizing settings to featured product #2114
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
kmeleta
force-pushed
the
constrain-featured-product
branch
from
1383117 to
f5e8b07
Compare
kmeleta
dismissed stale reviews from duygukalaycioglu and ludoboludo
via
23d9287
kmeleta
force-pushed
the
constrain-featured-product
branch
2 times, most recently
from
23d9287 to
9fb054e
Compare
kmeleta
force-pushed
the
constrain-featured-product
branch
from
9fb054e to
a2ab3e3
Compare
ludoboludo
approved these changes
Nov 22, 2022
| @@ -83,7 +95,9 @@ | |||
| loop: section.settings.enable_video_looping, | |||
| modal_id: section.id, | |||
| xr_button: false, | |||
| media_width: 0.5 | |||
| media_width: media_width, | |||
There was a problem hiding this comment.
It'd be kinda nice if instead of having the white space when the height is constrained to the screen height, it would grow/shrink the product info columns within a threshold of sizes.
But that would not work super well maybe with the sliders on mobile.
There was a problem hiding this comment.
I believe the thought was that would be too "magic". Especially considering we offer the media width setting to manually control that. The other risk would be the potentially large amount of layout shift when changing variants if the variant images varied in dimension at all.
There was a problem hiding this comment.
Yeah we would have to rework a lot of things to make it work that way. I feel like both ways are "magical" though. Ill be curious to see how it's used and received by merchants 🤔
I think some great descriptive explanation of what Constrain media to screen height means and how it might impact things will be useful on our help docs.
There was a problem hiding this comment.
I did like things about that particular solution (if we're talking about the same thing), but it was ruled out for reasons beyond refactor. I'm not sure we can refactor our way out of the layout shift issue without making other weird assumptions about what determines column width in certain scenarios. Though obviously the line to draw between what is too magic is subjective and ultimately on us to decide and validate with merchant frustration.
Rabter1
added a commit
to Rabter1/dawn-icletta
that referenced
this pull request
Nov 23, 2022
commit dc81806 Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com> Date: Tue Nov 22 13:32:46 2022 -0700 Add media sizing settings to featured product (Shopify#2114) commit 9875aea Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com> Date: Tue Nov 22 13:17:55 2022 -0700 Adjust constrain media values for quick add modal (Shopify#2113) * Adjust constrain media values for quickadd * Adjust offset logic * Minor formatting fix commit 2eb8bf6 Author: Ludo <ludo.segura@shopify.com> Date: Tue Nov 22 11:50:50 2022 -0500 Add fixed sizes for some images to prevent errors (Shopify#2087) * remove the mention of sizes where unnecessary * remove another instance * add fixed size to the images in structured data * adjust size to 1920 * edit logo size in structured data * remove conditional info * change value to match other logo size values commit 2f9b9f3 Author: Ludo <ludo.segura@shopify.com> Date: Mon Nov 21 15:44:24 2022 -0500 Remove alt attribute in image_tag filter where unnecessary (Shopify#2117) * Add fallback to alt attribute where image_tag filter is used * remove unnecessary use of alt attribute in image_tag filter commit 251e5d9 Author: Ludo <ludo.segura@shopify.com> Date: Mon Nov 21 12:18:44 2022 -0500 Move share button into a snippet (Shopify#2123) * Move share button into a snippet * change the property name to context and add details in snippet * address review comments * edit props names and description * Address reviewers comment. Remove unused prop and edit existing ID * move script at the top commit 539af27 Author: Eugene Kasimov <105315663+eugenekasimov@users.noreply.github.com> Date: Mon Nov 21 09:01:58 2022 -0800 Prettier liquid files snippets. (Shopify#2115) commit 503fdf8 Author: Eugene Kasimov <105315663+eugenekasimov@users.noreply.github.com> Date: Thu Nov 17 08:51:21 2022 -0800 Remove noscript css import for product recommendations (Shopify#2101) commit b2b01fd Author: Eugene Kasimov <105315663+eugenekasimov@users.noreply.github.com> Date: Thu Nov 17 08:50:33 2022 -0800 Change variables names (Shopify#2055) commit cf05d0d Author: Francisca Calabria Rubio <13103124+FCalabria@users.noreply.github.com> Date: Thu Nov 17 16:50:55 2022 +0100 Select text input content on trapFocus (Shopify#2106) * Select text input content on trapFocus * PR fix commit b86d1f3 Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com> Date: Thu Nov 17 08:50:43 2022 -0700 Change thumbnail media fit to "fill" by default (Shopify#2044) * Change thumbnail media fit to cover by default * Simplify and cleanup thumbnail fit approach commit 8ab8e61 Author: Ludo <ludo.segura@shopify.com> Date: Thu Nov 17 10:50:28 2022 -0500 Tweak the image_url size to be the max value where necessary (Shopify#2110) * Tweak the image_url size to be the max value where necessary * fix sizing in the header. commit c337301 Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Date: Thu Nov 17 10:49:52 2022 -0500 Update 1 translation file (Shopify#2130) Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> commit bc8f433 Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com> Date: Thu Nov 17 08:49:10 2022 -0700 Add constrain media and media fit settings to product page (Shopify#2052) * Add constrain height setting * Add constrain scaling functionality * Add media fit setting and functionality * Quick add override and typo fixes * Update setting language and code cosmetics * Update 8 translation files * Update 5 translation files * Update 3 translation files * Update 3 translation files * Update 1 translation file * Add comment Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
phapsidesGT
pushed a commit
to Gravytrain-UK/gt-shopify-dawn-theme
that referenced
this pull request
Sep 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR Summary:
Brings two new settings from the product page, aimed at improving media sizing flexibility, to the featured product section.
Important: The default media width has changed and results on a visual change
Why are these changes introduced?
Fixes #2024
This is a follow up to #2052 that adds the same main product
Constrain media to screen heightandMedia fitsettings to the Featured product section. This PR also adds the existing main productDesktop media widthsetting (which has already been released) to Featured product.What approach did you take?
If needed, general context around these new settings can be found there and in #2052 and linked issues.
Settings schema
The settings schema was copied directly main-product.liquid and added to featured-product.liquid and currently translated strings are directly referencing the main-product entries. I'm not sure it is necessary to duplicate all those identical strings, though there's probably an opportunity to create shared product entry that main and featured can reference for identical settings.
Duplicate media gallery liquid
Some liquid code for assigning media width values and passing the new settings to product-thumbnail.liquid has effectively been duplicated from product-media-gallery.liquid because featured product doesn't yet utilize that snippet. The addition of the media width settings here does bring the media gallery usage in featured-product closer to parity with the main product instance though, so I expect we'll be able to remove this and the existing duplicated code in the future.
Constrained and minimum height values
Right now, featured product is just inheriting the same values for
--viewport-offsetand--constrained-min-heightfrom main product. I found these to work reasonably with and without theSecondary backgroundsetting applied, but we can easily add overrides for featured product as needed.Other considerations
Changes to the media width
With the addition of the media width setting options (small 45%, medium 55%, and large 65%) there is no longer a 1:1 equivalent for the hardcoded 50% width featured product was originally using. Unlike main product, which defaults to "Large", featured product will default to "Medium" so the default appearance is closer to what it was. The result will be a slight visual change in default behavior.
Visual impact on existing themes
The default media width has been slightly increased from 50% of the available space to 55% (medium). There is no equivalent value and no way to avoid this visual change.
If the constrain is unchecked and media fit is set to the default "original" setting, no other visual changes should be observable.
Testing steps/scenarios
Demo links
Checklist