Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Tweak the image_url size to be the max value where necessary #2110

Merged
merged 2 commits into from
Nov 17, 2022
Merged

Tweak the image_url size to be the max value where necessary #2110

merged 2 commits into from
Nov 17, 2022

Conversation

ludoboludo
Copy link
Contributor

PR Summary:

This change affect the logo in the header (in the password page template and home page) as well as for the image banner images.
We were limiting the images' size wrongfully. This should output images as initially intended.

Why are these changes introduced?

Image in image banner section can get a little blurry even though the original is big enough to be sharp.

What approach did you take?

In a few places we were setting image_url: width: xxx to the wrong size. It should be using the maximum size we're setting in the srcset attribute.

Other considerations

N/A

Decision log

# Decision Alternatives Rationale Downsides
1

Visual impact on existing themes

Very small, if anything, sharper images.

Testing steps/scenarios

  • Add an image for the image banner section and check out the network tab to see which version of the image is loaded. It used to go up to a max of 1500 when it should be a max of 3840. Video explanation

Demo links

Checklist

@kmeleta kmeleta self-requested a review November 8, 2022 21:44
@kmeleta kmeleta self-assigned this Nov 8, 2022
eugenekasimov
eugenekasimov reviewed Nov 9, 2022
View reviewed changes
eugenekasimov
eugenekasimov approved these changes Nov 9, 2022
View reviewed changes
Copy link
Contributor

@eugenekasimov eugenekasimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me summarize what we have concluded here for the future just in case we need it.

  • When we use a filter image_url we need to specify either a width or height parameter.
  • The width parameter in our case will limit a maximum width that can be rendered/loaded via url.
  • We don't want to use the original size of an image because it may be bigger than 5760px which leads to an error in Liquid.
  • We decided to use the biggest value from the widths set that we provide as parameter for image_tag.

All looks good to me 🚀

kmeleta
kmeleta approved these changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the original issue and aligned the approach 👍

Thought I noticed another sizes issue while testing but I'm struggling to reproduce it. I'm thinking now it was just an editor effect https://screenshot.click/10-42-ksbkc-nhrk4.mp4

@ludoboludo ludoboludo self-assigned this Nov 10, 2022
@ludoboludo ludoboludo merged commit 8ab8e61 into main Nov 17, 2022
@ludoboludo ludoboludo deleted the fix-image-banner-width branch November 17, 2022 15:50
chavdar-ang pushed a commit to chavdar-ang/lermitage that referenced this pull request Nov 20, 2022
@ludoboludo @chavdar-ang
Tweak the image_url size to be the max value where necessary (Shopify…
c60952f 
…#2110)

* Tweak the image_url size to be the max value where necessary

* fix sizing in the header.
Rabter1 added a commit to Rabter1/dawn-icletta that referenced this pull request Nov 23, 2022
@Rabter1
Squashed commit of the following:
2592c41 
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
@ludoboludo
Tweak the image_url size to be the max value where necessary (Shopify…
ad5bf5d 
…#2110)

* Tweak the image_url size to be the max value where necessary

* fix sizing in the header.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmeleta kmeleta kmeleta approved these changes

@eugenekasimov eugenekasimov eugenekasimov approved these changes

Assignees

@ludoboludo ludoboludo

@kmeleta kmeleta

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ludoboludo @kmeleta @eugenekasimov