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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add constrain media and media fit settings to product page #2052
Add constrain media and media fit settings to product page #2052
Changes from 4 commits
ed4b4afa1321dccff4f206ab47ff7bd9b03e465f333091b9dfd9271068c37a35202bf354bd15dFile filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little nitpick. This could be moved before the media query
@media screen and (min-width: 750px) {on line1387🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this and next comment could be address to tidy up the stylesheet 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, moving that made me want to move other stuff. I struggled to find an order I liked for these things. It should have more of a base > mobile > desktop order to it now. Should be safe enough but wouldn't hurt to give things a quick run through to make sure the reordering didn't expose any specificity traps 7bd9b03
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to prefer having conditional classes at the end rather than in the middle. I find the whole thing easier to read.
Like this:
Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never aligned on this formally as a team. I don't feel too strongly about pushing back in this particular case, so I've made the change but I'm not always sold on basing the order of classes on the arbitrary decision to include an inline conditional or not.
gradientfor a example is a much more generic global class and we usually like to order those after more specific classes. Again it's a bit of a toss up here if gradient and media-settings are more generic than constrain-height, but that's how I think about it.class=”grid grid--2-col has-border show-heading-or-something slider {% if show_slider_desktop %} slider–desktop{% endif %}{% if some_setting %} grid--1-col{% endif %}”Also consider this ^ example,
grid--1-colgets moved to the end and is now harder to associate with other relevant related classes. I don't like that and that doesn't make this code any easier to understand in the editor or in the browser inspectorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but something I'd like to bring up regarding previous context from product media just to ensure we log this as a future decision in case we're overriding a past UX decision.
I remember we decided back in 2019 when 3D models were introduced to always display them in 1:1 aspect ratio. The rationale was that Shopify automatically generates a square poster image for uploaded 3D models, so this would ensure a smooth transition between the poster and the model's initial position. With the changes on this PR, 3D models can now be seen in any aspect ratio when the
Media fit: Fillis enabled. This leads to a less smooth transition as you can see in the video below.08-19-7jpi3-8bdmc.mp4
Because this is a setting and we're not changing the default behaviour, I think this is probably fine. If we were to force 3D models to be square would add a layer of "magic" to the setting and make it less clear.
I could not find the documentation to support this "always use square" principle, though, so I will cc @melissaperreault to see if she has any context from that time that could be relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default we preserve the existing 1:1 ratios for models, but you are correct, when using media fit: fill there are scenarios we they can diverge from that. I believe these cases a fairly minimal as I understand, but I'll outline them below for clarity. Let me know if I'm missing any.
media_fit: fillanddesktop_layout: 2 columnsand model shares a row with a portrait image. If the other image on the same row is a landscape or also square, the model media shouldn't crop. Particularly if 1:1 is the most common product image ratio, this should be an infrequent combination.media_fit: fillandconstrain: trueand viewport is small enough to constrain even square images. This can happen on tablet/desktop whenever models render fullwidth in the media column. I don't think this feel this will be super common either, but I do I think this is more likely to occur than the other scenario, especially if we were to default the constrain setting to true/on.Just want to mention this is an option of course, particularly for cases when the models are fullwidth (anything but desktop layout: 2 columns), but it would indeed be an unpredictable exception to the setting, which as I understand is our main issue with it. I also don't love the idea of doing this for the 2 column layout anyway just because it would create the vertical whitespace that the media fit setting specifically provides an option to prevent.