Shopify / dawn

Shopify's first source available reference theme, with Online Store 2.0 features and performance built-in.
Other
2.51k stars 3.37k forks source link

Bug: Pagination is broken in sections/main-list-collections.liquid #1996

Open mitchclarkson opened 2 years ago

mitchclarkson commented 2 years ago

Introduced in #1745.

Steps to reproduce

  1. Go to the Collections list template in the theme editor.
  2. Click the Collections list page section.
  3. Under Sort collections by:, select an option other than Alphabetically, A-Z.

Observed behaviour

Collections are sorted correctly and pagination produces the expected number of pages, but all collections are output on each page. Oddly, more than 50 collections can be output on each page (I tested up to 51 collections), even though for loops are supposed to have a limit of 50 iterations per page.

Expected behaviour

Collections would be sorted correctly and pagination would produce the expected number of pages, but collections would be paginated correctly across each page.

What's causing the issue?

Lines 6–24 of sections/main-list-collections.liquid are:

  {%- liquid
    case section.settings.sort
      when 'products_high' or 'products_low'
        assign collections = collections | sort: 'all_products_count'
      when 'date' or 'date_reversed'
        assign collections = collections | sort: 'published_at'
    endcase

    if section.settings.sort == 'products_high' or section.settings.sort == 'date_reversed' or section.settings.sort == 'alphabetical_reversed'
      assign collections = collections | reverse
    endif

    assign moduloResult = 28 | modulo: section.settings.columns_desktop
    assign paginate_by = 30
    if moduloResult == 0
      assign paginate_by = 28
    endif
  -%}
  {%- paginate collections by paginate_by -%}

I think the issue is caused by a combination of assigning the global collections object to a variable of the same name, and using this variable in the paginate tag.

Out of interest, I renamed the variable and received this error, Liquid error (sections/main-list-collections.liquid line 24): Array '_collections' is not paginateable., which leads me to believe the paginate tag is still using the global collections object while we're looping through the sorted collections in the local variable.

ludoboludo commented 1 year ago

This was fixed in one of the recent update we did 👍 Thanks for creating the issue and letting us know.

kmeleta commented 1 year ago

Re-opening this. Issue was reported again and I was able to reproduce on Dawn 8.0.0 with 50+ collections when sorted by any other method than A-Z.

ludoboludo commented 1 year ago

I think that might be something we shouldn't have introduced 🤔 That sorting option for the collections. Cause it doesn't seem like paginate will work with it. So we could do our own custom pagination in JS. Not sure if it's worth the effort though. When I look at other themes I think it's the same issue. Debut doesn't paginate, I see some other themes limiting the amount of collections to 48

alibosworth commented 1 month ago

This issue is still present in August 2024 @kmeleta @ludoboludo any insight into this? Is there an internal ticket to deal with the underlying issue that causes this?