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

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

Open
mitchclarkson opened this issue Sep 29, 2022 · 4 comments
Open
Labels
Category: Bug Something isn't working

Comments

@mitchclarkson
Copy link

mitchclarkson commented Sep 29, 2022

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
Copy link
Contributor

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

@kmeleta
Copy link
Contributor

kmeleta commented Feb 27, 2023

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.

@kmeleta kmeleta reopened this Feb 27, 2023
@kmeleta kmeleta added the Category: Bug Something isn't working label Feb 27, 2023
@ludoboludo
Copy link
Contributor

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
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants