Add fixed sizes for some images to prevent errors #2087

ludoboludo · 2022-11-01T21:15:44Z

PR Summary:

In a few places we're passing in the width of our images as an argument for the image_url filter. This can create issues as there is a limit being checked and images can't be wider than 5760.

So I'm adding a fixed width to those so we're not running into an issue.

Why are these changes introduced?

Fixes #1893

What approach did you take?

Added a fixed size, Google states: Images must be at least 160×90 pixels and at most 1920×1080 pixels.

Other considerations

On the issue there was some chat about the filter itself applying a limit to size that can be passed.

Decision log

#	Decision	Alternatives	Rationale	Downsides
1

Visual impact on existing themes

No visual impact expected.

Testing steps/scenarios

I haven't been able to replicate the issue brought up as I wasn't able to upload an image that was wider than 5760. I posted on the issue to see if I'm missing something in my replication steps.

Step 1

Demo links

Editor

Checklist

Added PR summary for release notes
Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
Followed theme code principles
Linted with Theme Check
Tested on mobile
Tested on multiple browsers
Tested for accessibility

bakura10 · 2022-11-02T16:48:45Z

Has the change to make the width argument optional been merged already ?

ludoboludo · 2022-11-02T17:19:57Z

@bakura10 From the sound of it, it always was the case but I'm a bit skeptical. Which is why I had not asked for reviews yet. I should change the status to draft for now as I would ideally want to have our docs updated to reflect the change as well. So we don't mention its requirement.

bakura10 · 2022-11-02T19:34:55Z

100% sure it was not the case :D. The image_url used to fail if you did not pass a width attribute so it would be nice to have a confirmation indeed :)

ludoboludo · 2022-11-03T19:01:35Z

Yeah you're right it's still failing. I'm getting Liquid error (sections/main-product.liquid line 58): image_url must define width or height.

Ill change the PR to use a width of 1200

eugenekasimov

Hey Ludo. I think your approach fixes the current issue. I left a minor comment about my concern.

Based on what I have learned about Schema.org it may be a good idea to have a few alternative versions of images (different resolutions and/or aspect ratios). It can improve a search result for pages. It can be as a follow up PR though. Let me know what you think about it. Thanks.

eugenekasimov · 2022-11-07T23:08:27Z

sections/main-article.liquid

          "height": {{ settings.share_image.height | json }},
-          "url": {{ settings.share_image | image_url: width: settings.share_image.width | prepend: "https:" | json }},
+          "url": {{ settings.share_image | image_url: width: 1920 | prepend: "https:" | json }},
          "width": {{ settings.share_image.width | json }}


I'm not sure for 100% but I can assume that "height": and "width": may lead to the same error in our case. I would remove them, or leave only one of them with fixed value. What do you think?

I'm not sure I follow.

or leave only one of them with fixed value

Isn't it what I'm doing in the PR already ^ ?

As for adding multiple images, yeah I think it could be done as a follow up if we see it's necessary 👍

I'm sorry Ludo, it seems like I'm bad at explaining. My concern is next:

line 281 - you provide url with fixed width: 1920.

line 282 - you provide width with value settings.share_image.width that may pass an original size of an image and it can be 5000px or bigger.

So, there shouldn't be an error in Liquid as it was before this PR but I just wanna be sure that there is no conflict or potential issue for merchants for their search optimization. Because I don't know what happens in case when the actual image is pulled through the url has width 1920px, but at the same time we are telling Schema.org to use width: settings.share_image.width which can be 5000px for example.

Oh I see what you mean 👍 good point, thanks for bringing it up 🙂

Makes me realize that this part is never coming up as there isn't any settings.share_image on the theme. Must have been added by mistake to replicate a previous behaviour. So I'll just remove it for now 👍
Doesn't seem to be the most important piece of information anyway when I look at the article structured data guidelines.

ludoboludo · 2022-11-08T19:10:09Z

sections/main-article.liquid

-          "url": {{ settings.share_image | image_url: width: settings.share_image.width | prepend: "https:" | json }},
-          "width": {{ settings.share_image.width | json }}
-        },
-      {% endif %}


Removed this as it's not outputting ever as a share_image setting doesn't exists.

eugenekasimov

Looking good 🚀

sections/header.liquid

commit dc81806 Author: Ken Meleta <[email protected]> Date: Tue Nov 22 13:32:46 2022 -0700 Add media sizing settings to featured product (Shopify#2114) commit 9875aea Author: Ken Meleta <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> Date: Mon Nov 21 09:01:58 2022 -0800 Prettier liquid files snippets. (Shopify#2115) commit 503fdf8 Author: Eugene Kasimov <[email protected]> Date: Thu Nov 17 08:51:21 2022 -0800 Remove noscript css import for product recommendations (Shopify#2101) commit b2b01fd Author: Eugene Kasimov <[email protected]> Date: Thu Nov 17 08:50:33 2022 -0800 Change variables names (Shopify#2055) commit cf05d0d Author: Francisca Calabria Rubio <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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>

* 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

ludoboludo added 2 commits November 1, 2022 12:02

remove the mention of sizes where unnecessary

bc15ca7

remove another instance

707b8fc

ludoboludo marked this pull request as draft November 2, 2022 17:33

ludoboludo self-assigned this Nov 3, 2022

ludoboludo added 2 commits November 4, 2022 10:54

add fixed size to the images in structured data

397a412

adjust size to 1920

e8380a1

ludoboludo marked this pull request as ready for review November 4, 2022 15:28

edit logo size in structured data

f4951ee

ludoboludo changed the title ~~remove the mention of sizes where unnecessary~~ Add fixed sizes for some images to prevent errors Nov 4, 2022

eugenekasimov self-requested a review November 7, 2022 20:08

eugenekasimov reviewed Nov 7, 2022

View reviewed changes

remove conditional info

d75bc3a

ludoboludo commented Nov 8, 2022

View reviewed changes

ludoboludo requested a review from eugenekasimov November 8, 2022 19:13

eugenekasimov previously approved these changes Nov 8, 2022

View reviewed changes

metamoni reviewed Nov 9, 2022

View reviewed changes

sections/header.liquid Outdated Show resolved Hide resolved

change value to match other logo size values

27a44e3

ludoboludo dismissed eugenekasimov’s stale review via 27a44e3 November 9, 2022 15:54

ludoboludo requested review from eugenekasimov and metamoni November 9, 2022 15:54

eugenekasimov approved these changes Nov 9, 2022

View reviewed changes

metamoni approved these changes Nov 22, 2022

View reviewed changes

ludoboludo merged commit 2eb8bf6 into main Nov 22, 2022

ludoboludo deleted the img-url-remove-image-width branch November 22, 2022 16:50

Add fixed sizes for some images to prevent errors #2087

Add fixed sizes for some images to prevent errors #2087

Uh oh!

Conversation

ludoboludo commented Nov 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary:

Why are these changes introduced?

What approach did you take?

Other considerations

Decision log

Visual impact on existing themes

Testing steps/scenarios

Demo links

Checklist

Uh oh!

bakura10 commented Nov 2, 2022

Uh oh!

ludoboludo commented Nov 2, 2022

Uh oh!

bakura10 commented Nov 2, 2022

Uh oh!

ludoboludo commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eugenekasimov left a comment

Choose a reason for hiding this comment

Uh oh!

eugenekasimov Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ludoboludo Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

eugenekasimov Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

ludoboludo Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

ludoboludo Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

ludoboludo Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

eugenekasimov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ludoboludo commented Nov 1, 2022 •

edited

Loading

ludoboludo commented Nov 3, 2022 •

edited

Loading

eugenekasimov Nov 7, 2022 •

edited

Loading