Added page width setting and fixed image quality #292

tyleralsbury · 2021-07-29T20:19:54Z

Why are these changes introduced?

Fixes #289
Fixes #291

What approach did you take?

I've added a setting in the schema and updated the srcset/sizes properties for the images throughout the theme to output the right quality images for each setting.

Other considerations

Might need a content pass from @melissaperreault and @gretchen-willenberg - I put in what I thought made sense, but there may be a better way of phrasing things. Also note the position within the theme settings menu which can be changed if it's not in a great spot.

Currently there are two options - 1600px and 1200px, but we could technically add more or use a range. I implemented it in a way that allows for either approach.

Demo links

Store
Editor

Checklist

martinamarien

I didn't get a chance yet to review the actual image size changes. I will do this tomorrow if you don't already have two reviewers.

config/settings_schema.json

locales/en.default.schema.json

martinamarien · 2021-07-30T19:17:27Z

assets/base.css

@@ -181,7 +181,7 @@ html.no-js .no-js-hidden {
  }

  .page-width-desktop {
-    max-width: 160rem;
+    max-width: var(--page-width);


Question: Should the page-width--narrow be adjusted based on which page with a merchant selects?

Good question - @wiktoriaswiecicka do you think it should be something like a percentage of the selected value? Looks like it was a kinda strange width of 726px before. If we made it 72rem, that'd be 60%. So maybe we could make it so that it's 60% of the width of the page-width.

Then when it's 1200px, it'll be 720px for --narrow and when it's 1600px, it'll be 960px for the --narrow. What do you think?

I agree - the percentage value (60%) makes sense. 👍

sections/collage.liquid

sections/collection-list.liquid

martinamarien · 2021-07-30T19:35:37Z

snippets/product-thumbnail.liquid

+            {% if media.preview_image.width >= 1100 %}{{ media.preview_image | img_url: '1100x' }} 1100w,{% endif %}
+            {% if media.preview_image.width >= 1500 %}{{ media.preview_image | img_url: '1500x' }} 1500w{% endif %}"
+          src="{{ media | img_url: '1500x' }}"
+          sizes="(min-width: {{ settings.page_width }}px) {{ settings.page_width | minus: 100 | times: 0.64 | round }}px, (min-width: 750px) calc((100vw - 11.5rem) / 2), calc(100vw - 4rem)"


Can you explain why you chose 0.64?

Yep - the width of the product page media wrapper is 64%, so this allows a slightly more accurate calculation of how big the image should be.

gretchen-willenberg · 2021-08-02T15:25:08Z

Back from break with some belated copy feedback:

Would spell out maximum to benefit translation and accessibility: Maximum page width
Is px included with 1200 and 1600? 1200px and 1600px without spaces is correct.
For consistency and future growth, would Pages be a subhead to give more information on what the layout is applied to vs just the settings label? If Layout was like Typography, etc:
Layout
Pages
Maximum page width

tauthomas01

I looked at the image rendering and the network tab, images size look adequate.
I left some small comments.

Let me know if you have any questions

sections/image-with-text.liquid

tauthomas01 · 2021-08-02T15:49:55Z

sections/main-search.liquid

-                                  {%- if item.image.src.width >= 940 -%}{{ item.image.src | img_url: '940x' }} 714w{%- endif -%}"
-                              src="{{ item.image.src | img_url: '533x' }}"
+                                  {%- if item.image.src.width >= 940 -%}{{ item.image.src | img_url: '940x' }} 714w{%- endif -%}
+                                  {%- if item.image.src.width >= 1066 -%}{{ item.image.src | img_url: '1066x' }} 1066w{%- endif -%}"


I am curious where does 1066 comes from but also this template.

Because the grid will always be set at 4 columns, one grid item measures 264px. If we want the high resolution size (like high density display, which I believe is 2x, or 3x of density pixel ratio if it's iPhone X or high-end phone), it would be 264 * 3 = 792px.

The 1066 comes from a 3x at 749px. Rather than base it on devices, which change over time, I've based it on the actual breakpoints. The images are 355, so 355 * 3 = 1065 (not 1066, so that was my bad).

sections/main-search.liquid

tauthomas01 · 2021-08-02T15:54:11Z

sections/video.liquid

+              {%- if section.settings.cover_image.width >= 1780 -%}{{ section.settings.cover_image | img_url: '1780x' }} 1780w,{%- endif -%}
+              {%- if section.settings.cover_image.width >= 2000 -%}{{ section.settings.cover_image | img_url: '2000x' }} 2000w,{%- endif -%}
+              {%- if section.settings.cover_image.width >= 3000 -%}{{ section.settings.cover_image | img_url: '3000x' }} 3000w,{%- endif -%}
+              {%- if section.settings.cover_image.width >= 3840 -%}{{ section.settings.cover_image | img_url: '3840x' }} 3840w,{%- endif -%}"


I'm curious about this one as well. The max width an image can take is 1500px and on high end laptop, the density pixel ratio is 2x, so 1500 x 2 = 3000 so I'm wondering if the 3840px is needed.

I am not sure if it makes sense.

Since the video section can go full width, I've included 3840 to account for 4k screens. I was going to also include 7680, but I don't think there are 2x pixel density 4k screens.

martinamarien · 2021-08-03T13:14:00Z

For consistency and future growth, would Pages be a subhead to give more information on what the layout is applied to vs just the settings label? If Layout was like Typography, etc:
Layout
Pages
Maximum page width

@gretchen-willenberg Since Pages is a type of template, is there any concern that merchants might think this setting would only apply to the Pages template type if we use the Pages subheading?

wiktoriaswiecicka-zz · 2021-08-03T15:00:31Z

For consistency and future growth, would Pages be a subhead to give more information on what the layout is applied to vs just the settings label? If Layout was like Typography, etc:
Layout
Pages
Maximum page width

@gretchen-willenberg Since Pages is a type of template, is there any concern that merchants might think this setting would only apply to the Pages template type if we use the Pages subheading?

I agree, that using Maximum page width as a label may sound confusing. I'd be in favour of keeping Maximum width, or maybe Maximum grid width?

gretchen-willenberg · 2021-08-03T15:34:25Z

I agree, that using Maximum page width as a label may sound confusing. I'd be in favour of keeping Maximum width, or maybe Maximum grid width?

+100 @martinamarien and @wiktoriaswiecicka

If layout needed a subheading for context it would have to be something more inclusive to reflect all templates and pages, like online store pages Since it's Theme settings > layout we likely don't need unless or until we add any other layout settings.
Agree with label of maximum width. Using grid might be too inside-design and a bit of a learning curve for merchants that we may not need to put on them.

tyleralsbury · 2021-08-03T15:54:03Z

Made some adjustments based on the comments + discussions. Let me know if this is good for the content or if we want to make further changes:

gretchen-willenberg · 2021-08-03T16:07:55Z

Thank you, @tyleralsbury! Please delete the Pages subheading.

Sorry for the confusion.

tyleralsbury · 2021-08-03T17:22:10Z

No problemo. Removed it.

tauthomas01

It looks good on my end. We just need to fix this small typo.

sections/main-search.liquid

tyleralsbury · 2021-08-03T19:57:01Z

Fixed them, @tauthomas01 - good catch. It was hard to find all of those.

tauthomas01

LGTM

martinamarien · 2021-08-04T17:29:11Z

sections/collage.liquid

+                          {%- if block.settings.cover_image.width >= 1100 -%}{{ block.settings.cover_image | img_url: '1100x' }} 1100w,{%- endif -%}
+                          {%- if block.settings.cover_image.width >= 1500 -%}{{ block.settings.cover_image | img_url: '1500x' }} 1500w,{%- endif -%}
+                          {%- if block.settings.cover_image.width >= 2200 -%}{{ block.settings.cover_image | img_url: '2200x' }} 2200w,{%- endif -%}
+                          {%- if block.settings.cover_image.width >= 3000 -%}{{ block.settings.cover_image | img_url: '3000x' }} 3000w,{%- endif -%}"


In some of the srcset values, you're ending with a ,. I think we can remove it in the last if block - there are a few files that this is happening in.

I don't think that causes any issues with the srcset working properly.

I kinda bounced between the two the past few times I looked at srcset and I actually think the trailing comma is better because if someone wants to add another element to the end of it, they can, whereas if the comma's not there they would need to notice that the comma is missing, add it, and then add their line.

So if anything, I think they should have the comma. I'm not sure if we should worry too much about it now, though. We can smooth out rough edges like this in the coming weeks as we tackle some of the lower priority fixes.

martinamarien

I tested all the use cases and it looks good.
Just one question for you about a src size

martinamarien · 2021-08-04T18:12:30Z

sections/main-collection-banner.liquid

+            {%- if collection.image.width >= 535 -%}{{ collection.image | img_url: '535x' }} 535w,{%- endif -%}
+            {%- if collection.image.width >= 750 -%}{{ collection.image | img_url: '750x' }} 750w,{%- endif -%}
+            {%- if collection.image.width >= 1070 -%}{{ collection.image | img_url: '1070x' }} 1070w,{%- endif -%}
+            {%- if collection.image.width >= 1500 -%}{{ collection.image | img_url: '1500x' }} 1500w,{%- endif -%}"
          src="{{ collection.image | img_url: '533x' }}"


Question: Why '533x'? In the other images, it looks like src is using a value represented by the srcset

Ah, good question. I missed that one. I've been trying to represent the largest 1x size for the most part since I'm assuming anyone using a browser that doesn't support srcset is unlikely to be on any sort of fancy high pixel density screen. I'll fix it up.

gregjotau · 2021-08-04T22:25:21Z

It might be because of changes we have on our theme, but I immediately think that merging this change into our theme made it look worse 😅

https://github.com/gregjotau/dawn-famme
famme.no

gregjotau · 2021-08-04T22:26:28Z

But might need we need to reupload and adjust our images based on these changes? 🤔

martinamarien · 2021-08-04T22:53:26Z

but I immediately think that merging this change into our theme made it look worse 😅

Do you mean the page width setting or the changes to the image sizes/srcset?

If it's the page width setting, you can choose to change the setting to 1200, which is what dawn's previous width was by default.

* Added page width setting and fixed image quality

gregjotau · 2021-08-05T13:26:23Z

It might be the changes to the sizes which made it a bit weird on larger screens (24"), but it does not matter much since everyone browses on mobile.

We do have some customization for mobile photo upload in the image banner liquid, so might be that we have screwed up something as well 😓 Will have to do a diff to verify.

A built in mobile image upload would be awesome so we can remove our customization 🙌

* main: (23 commits) Cart content update (Shopify#370) Footer spacing and alignment adjustments (Shopify#369) Product Template UI polish updates (Shopify#219) footer ui updates (Shopify#318) Fix cart improvements empty state (Shopify#319) [Announcement] Adjust block id for displaying dynamic names (Shopify#327) Update translations: buyer (Shopify#329) Revert editor setting changes (Shopify#328) Update translations (Shopify#294) Fix collection filtering UX (Shopify#268) Added page width setting and fixed image quality (Shopify#292) Add top border on cart notification when "show separator line" setting is deactivated (Shopify#306) movebadge code into base.css (Shopify#313) Collage UI bug fixes (Shopify#308) Customer account UI polish (Shopify#177) Added custom liquid block to product page (Shopify#269) Fix disclosure icon (Shopify#310) Fix facet price filter label spacing (Shopify#300) Fix duplicate search icon when header logo is set to "Top center" (Shopify#252) Add card outline setting (Shopify#239) ...

* Added page width setting and fixed image quality

tyleralsbury added 3 commits July 29, 2021 16:16

Added page width setting and fixed image quality

c37e977

Fixed password and gift card

a8d8429

Article card fix

69e848d

tyleralsbury requested a review from melissaperreault July 29, 2021 20:37

tauthomas01 self-assigned this Aug 2, 2021

martinamarien reviewed Aug 2, 2021

View reviewed changes

tauthomas01 reviewed Aug 2, 2021

View reviewed changes

melissaperreault requested review from wiktoriaswiecicka-zz and removed request for melissaperreault August 2, 2021 18:28

Adjustments

1273800

tyleralsbury requested review from tauthomas01 and martinamarien August 3, 2021 15:54

Remove subheading

6cb3e11

tauthomas01 reviewed Aug 3, 2021

View reviewed changes

sections/main-search.liquid Outdated Show resolved Hide resolved

sections/main-search.liquid Outdated Show resolved Hide resolved

Fixed misaligned size

85f82c7

tyleralsbury requested a review from tauthomas01 August 3, 2021 19:41

tauthomas01 previously approved these changes Aug 3, 2021

View reviewed changes

martinamarien reviewed Aug 4, 2021

View reviewed changes

martinamarien previously approved these changes Aug 4, 2021

View reviewed changes

Fixed src

1fb480e

tyleralsbury dismissed stale reviews from martinamarien and tauthomas01 via 1fb480e August 4, 2021 18:59

tyleralsbury requested review from tauthomas01 and martinamarien August 4, 2021 18:59

tauthomas01 approved these changes Aug 4, 2021

View reviewed changes

martinamarien approved these changes Aug 4, 2021

View reviewed changes

tyleralsbury merged commit 9070ef0 into main Aug 4, 2021

tyleralsbury deleted the page-width-setting branch August 4, 2021 20:20

bredowmax mentioned this pull request Aug 5, 2021

Collection photo just became softer with latest update #322

Open

olafghanizadeh pushed a commit to hyttefeber/dawn that referenced this pull request Aug 5, 2021

Added page width setting and fixed image quality (Shopify#292)

5e2ddba

* Added page width setting and fixed image quality

phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024

Added page width setting and fixed image quality (Shopify#292)

19f8a73

* Added page width setting and fixed image quality

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added page width setting and fixed image quality #292

Added page width setting and fixed image quality #292

tyleralsbury commented Jul 29, 2021 •

edited

Loading

martinamarien left a comment

martinamarien Jul 30, 2021

tyleralsbury Aug 3, 2021

wiktoriaswiecicka-zz Aug 3, 2021

martinamarien Jul 30, 2021

tyleralsbury Aug 3, 2021

gretchen-willenberg commented Aug 2, 2021

tauthomas01 left a comment

tauthomas01 Aug 2, 2021 •

edited

Loading

tyleralsbury Aug 3, 2021

tauthomas01 Aug 2, 2021

tyleralsbury Aug 3, 2021

martinamarien commented Aug 3, 2021

wiktoriaswiecicka-zz commented Aug 3, 2021 •

edited

Loading

gretchen-willenberg commented Aug 3, 2021

tyleralsbury commented Aug 3, 2021

gretchen-willenberg commented Aug 3, 2021

tyleralsbury commented Aug 3, 2021

tauthomas01 left a comment

tyleralsbury commented Aug 3, 2021

tauthomas01 left a comment

martinamarien Aug 4, 2021

tyleralsbury Aug 4, 2021

martinamarien left a comment

martinamarien Aug 4, 2021

tyleralsbury Aug 4, 2021

gregjotau commented Aug 4, 2021

gregjotau commented Aug 4, 2021

martinamarien commented Aug 4, 2021

gregjotau commented Aug 5, 2021

Added page width setting and fixed image quality #292

Added page width setting and fixed image quality #292

Conversation

tyleralsbury commented Jul 29, 2021 • edited Loading

martinamarien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gretchen-willenberg commented Aug 2, 2021

tauthomas01 left a comment

Choose a reason for hiding this comment

tauthomas01 Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinamarien commented Aug 3, 2021

wiktoriaswiecicka-zz commented Aug 3, 2021 • edited Loading

gretchen-willenberg commented Aug 3, 2021

tyleralsbury commented Aug 3, 2021

gretchen-willenberg commented Aug 3, 2021

tyleralsbury commented Aug 3, 2021

tauthomas01 left a comment

Choose a reason for hiding this comment

tyleralsbury commented Aug 3, 2021

tauthomas01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinamarien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregjotau commented Aug 4, 2021

gregjotau commented Aug 4, 2021

martinamarien commented Aug 4, 2021

gregjotau commented Aug 5, 2021

tyleralsbury commented Jul 29, 2021 •

edited

Loading

tauthomas01 Aug 2, 2021 •

edited

Loading

wiktoriaswiecicka-zz commented Aug 3, 2021 •

edited

Loading