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.

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

update featured collection so we don't lazy load images above the fold #3741

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

katiepeterso
Copy link

@katiepeterso katiepeterso commented Mar 4, 2025

PR Summary:

The first row of the Feature Collection is visible above the fold in Rise and on some screen sizes in Dawn. Therefore, we should not lazy load those images. I conditionally set lazy_load to false for card-product's that are in the first row based on the number of columns to show per row.

Why are these changes introduced?

Images above the fold should not be lazy loaded. This is being introduced to follow Shopify's guidelines and to improve performance.

What approach did you take?

Other considerations

Decision log

# Decision Alternatives Rationale Downsides
1

Visual impact on existing themes

This should only improve the loading performance of the page, so the change may not be visually detectable. Here is a before and after measurement from Lighthouse:

Before After
Screenshot 2025-03-03 at 1 10 00 PM Screenshot 2025-03-03 at 1 08 16 PM
Screenshot 2025-03-03 at 1 10 09 PM Screenshot 2025-03-03 at 1 08 27 PM

Testing steps/scenarios

  • Step 1

Demo links

Checklist

@katiepeterso katiepeterso marked this pull request as draft March 4, 2025 15:53
@katiepeterso katiepeterso marked this pull request as ready for review March 4, 2025 16:58
@Al-Campuzano
Copy link
Contributor

Al-Campuzano commented Mar 4, 2025

@katiepeterso how could i tophat this change? there are some demo links in the PR description but they're not going where i thought they would

@@ -145,6 +150,7 @@
show_vendor: section.settings.show_vendor,
media_aspect_ratio: section.settings.image_ratio,
image_shape: section.settings.image_shape,
lazy_load: false,
Copy link
Author

Choose a reason for hiding this comment

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

This section (if shown) will only have one row of items, so lazy_load can always be false.

@@ -99,6 +105,10 @@
{%- if section.settings.collection.products.size > 0 -%}
{% paginate section.settings.collection.products by section.settings.products_to_show %}
{%- for product in section.settings.collection.products limit: section.settings.products_to_show -%}
{% assign lazy_load = false %}
{% if forloop.index > max_columns_to_show %}
Copy link
Author

Choose a reason for hiding this comment

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

I decided to use the max possible columns to show here rather than try to figure out which layout is displayed. The numbers should not be so different (like, same order of magnitude).

@katiepeterso
Copy link
Author

@katiepeterso how could i tophat this change? there are some demo links in the PR description but they're not going where i thought they would

I think I have the right URL's in the description now, @Al-Campuzano

@Al-Campuzano
Copy link
Contributor

@katiepeterso how could i tophat this change? there are some demo links in the PR description but they're not going where i thought they would

I think I have the right URL's in the description now, @Al-Campuzano

Thanks, Katie :)
I'm not really able to tell if this is working. Could you put before/after screenshots or short video please? 🙏

@katiepeterso
Copy link
Author

Thanks, Katie :)
I'm not really able to tell if this is working. Could you put before/after screenshots or short video please? 🙏

That's good! You should not see a change to the functionality. I have added before and after performance tests to the description.

@katiepeterso katiepeterso requested a review from a team March 10, 2025 15:36
@Al-Campuzano
Copy link
Contributor

Thanks, Katie :)
I'm not really able to tell if this is working. Could you put before/after screenshots or short video please? 🙏

That's good! You should not see a change to the functionality. I have added before and after performance tests to the description.

What about the actual attributes in the HTML, though? that should be visible, and i could not see it. Granted, i may not know what i'm doing 😅

@katiepeterso katiepeterso marked this pull request as draft March 10, 2025 15:49
@@ -99,6 +105,10 @@
{%- if section.settings.collection.products.size > 0 -%}
{% paginate section.settings.collection.products by section.settings.products_to_show %}
{%- for product in section.settings.collection.products limit: section.settings.products_to_show -%}
{% assign lazy_load = false %}
{% if forloop.index > max_columns_to_show %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that we assign this to true only once, we could adjust this?

Suggested change
{% if forloop.index > max_columns_to_show %}
{% if forloop.index > max_columns_to_show and lazy_load == false %}

Would save trying this on all products

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants