Update quick order list to use paginated product.variants #3710

lhoffbeck · 2025-01-28T15:09:56Z

PR Summary:

Updates quick order list component to paginate variants

Why are these changes introduced?

What this IS
Product.variants is capped to return a max of 250 objects, see docs for more details. As a result, the quick order list as-is can't contain a comprehensive view of all of a product's variants.

The main changes in this PR are to (1) paginate product.variants when rendering quick order list and (2) display pagination links within quick order list. There are a number of other minor changes to refactor and simplify the component.

What this IS NOT
While we did fix a number of existing bugs, this is NOT intended to be a full rebuild of the quick order list.

What approach did you take?

Code overview / brief demo: https://share.descript.com/view/DwFdTGzu2kM

Testing steps/scenarios

Currently only on spin while waiting for liquid pagination changes to land in production. If my spin link dies you can use this link to resume the instance. Note that requests are much slower than they are in prod.

Check that a product that doesn't have enough variants to paginate still works as intended
Check product with paginated variants
- Can add variants on first page
- Can load additional pages
- Can add variants on page other than first
- Remove all removes all variants across pages
Check quick order bulk still functions as intended
- Embedded quantity picker works
- Modal contains paginated quick order list

Demo links

https://shop1.shopify.online-store-web-csqt.lars-hoffbeck.us.spin.dev/products/sleek-steel-shirt

Checklist

Added PR summary for release notes
Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
Followed theme code principles
Linted with Theme Check
Tested on mobile
Tested on multiple browsers
Tested for accessibility

lhoffbeck · 2025-01-31T13:19:17Z

assets/global.js

+  setRequestStarted(requestStarted) {
+    this._requestStarted = requestStarted;
+  }
+
+  get requestStarted() {
+    return this._requestStarted;


Migrated class variable requestStarted to setRequestStarted/getRequestStarted functions and updated usage. This is the parent component, children were directly setting this.requestStarted. Less error prone / easier to determine what's happening by converting to funcs on the parent.

lhoffbeck · 2025-01-31T13:20:43Z

assets/global.js

-    const quickBulkElement = this.closest('quick-order-list') || this.closest('quick-add-bulk');
-    quickBulkElement.updateMultipleQty(items);


This was doing an unnecessary DOM call. BulkAdd is already subclassed by QuickOrderList and QuickAddBulk, so we can just call updateMultipleQty on the instance.

lhoffbeck · 2025-01-31T13:22:56Z

assets/global.js

@@ -1247,18 +1257,11 @@ class BulkAdd extends HTMLElement {
    } else {
      event.target.setCustomValidity('');
      event.target.reportValidity();
+      event.target.setAttribute('value', inputValue);


This enables us to treat the DOM as a source of truth rather than maintaining client state for all pending requests. See QuickOrderList.renderSections for how it's used.

lhoffbeck · 2025-01-31T13:23:38Z

assets/global.js

-  getSectionsUrl() {
-    if (window.pageNumber) {
-      return `${window.location.pathname}?page=${window.pageNumber}`;
-    } else {
-      return `${window.location.pathname}`;
-    }
-  }


This was only used by the QuickOrderBulk subclass, no need for it to be on the parent.

lhoffbeck · 2025-01-31T13:26:11Z

assets/quantity-popover.css

@@ -26,7 +26,7 @@ quantity-popover volume-pricing li {
 .quantity-popover__info .button-close,
 .variant-remove-total quick-order-list-remove-all-button .button,
 .quick-order-list-total__confirmation quick-order-list-remove-all-button .button,
-quantity-popover quick-order-list-remove-button .button {
+quantity-popover .quick-order-list-remove-button .button {


QuickOrderListRemoveButton was modeled as a web component that also subclassed BulkAdd. As a result, each QuickOrderListRemoveButton instance had its own quantity update queue and you could end up in funky UI states 😬

By migrating it to a simple button and adding an event listener to QuickOrderList, we're able to correctly queue events.

lhoffbeck · 2025-01-31T13:28:20Z

assets/quick-add-bulk.js

      listenForActiveInput() {
        if (!this.classList.contains('hidden')) {
-          this.getInput().addEventListener('focusin', (event) => event.target.select());
+          this.getInput()?.addEventListener('focusin', (event) => event.target.select());
        }
        this.isEnterPressed = false;
      }

      listenForKeydown() {
-        this.getInput().addEventListener('keydown', (event) => {
+        this.getInput()?.addEventListener('keydown', (event) => {
          if (event.key === 'Enter') {
-            this.getInput().blur();
+            this.getInput()?.blur();
            this.isEnterPressed = true;
          }


🐛-fix not related to this PR. This was throwing an error if quick add bulk was in a sold out state and no input was shown.

lhoffbeck · 2025-01-31T13:29:25Z

assets/quick-add-bulk.js

+            id: `quick-add-bulk-${this.dataset.index}-${this.closest('.collection-quick-add-bulk').dataset.id}`,
            section: this.closest('.collection-quick-add-bulk').dataset.id,
-            selector: `#quick-add-bulk-${this.dataset.id}-${this.closest('.collection-quick-add-bulk').dataset.id}`,
+            selector: `#quick-add-bulk-${this.dataset.index}-${this.closest('.collection-quick-add-bulk').dataset.id}`,


🐛-fix not related to this PR. QuickAddBulk doesn't have a data-id, this section was always getting ignored.

lhoffbeck · 2025-01-31T13:30:20Z

assets/quick-add-bulk.js

@@ -148,7 +153,7 @@ if (!customElements.get('quick-add-bulk')) {
          },
          {
            id: 'CartDrawer',
-            selector: '#CartDrawer',
+            selector: '.drawer__inner',


Minor refactor -- by targeting .drawer__inner rather than #CartDrawer we don't need to do this cart rebind because the content isn't overwritten.

lhoffbeck · 2025-01-31T13:32:30Z

assets/quick-add-bulk.js

-            setTimeout(() => {
-              document.querySelector('#CartDrawer-Overlay').addEventListener('click', this.cart.close.bind(this.cart));
-            });


This is pretty bad, we shouldn't declaratively rebind events in another component. We were able to remove it here by updating the targeting (above).

As a side note, I really dislike calling cart with a sections prop to fetch updates for sections outside the current component and then manually updating them in the response. Makes sense why it was done from an efficiency perspective, but the pattern I'd ideally like to see here is to publish an event that the cart got updated and any components that depend on cart refetch themselves.

lhoffbeck · 2025-01-31T13:38:48Z

assets/quick-order-list.css

Changes in this file were primarily to adjust table paddings to work with pagination, and migrate away from the quick-order-list-remove-button component to a simple button (see context).

lhoffbeck · 2025-01-31T13:39:20Z

assets/quick-order-list.css

@@ -334,6 +334,10 @@ quick-order-list-remove-button:hover .icon-remove {
  visibility: hidden;
 }

+.quick-order-list-total__info .loading__spinner:not(.hidden) + quick-order-list-remove-all-button {
+  visibility: hidden;
+}


This fixes a bug where the spinner next to the view cart button in the table footer would get positioned ~20px below center.

lhoffbeck · 2025-01-31T13:40:15Z

assets/quick-order-list.js

-if (!customElements.get('quick-order-list-remove-button')) {
-  customElements.define(
-    'quick-order-list-remove-button',
-    class QuickOrderListRemoveButton extends BulkAdd {
-      constructor() {
-        super();
-        this.addEventListener('click', (event) => {
-          event.preventDefault();
-          this.startQueue(this.dataset.index, 0);
-        });
-      }
-    }
-  );
-}


Replaced this with a simple button so that events would be processed in the same QuickOrderList queue as the other quantity updates.

lhoffbeck · 2025-01-31T13:43:14Z

assets/quick-order-list.js

-if (!customElements.get('quick-order-list-remove-all-button')) {
-  customElements.define(
-    'quick-order-list-remove-all-button',
-    class QuickOrderListRemoveAllButton extends HTMLElement {


This isn't actually removed, just moved after QuickOrderList to fix initialization order

lhoffbeck · 2025-01-31T13:47:13Z

assets/quick-order-list.js

-    }
-  );
-}
-
 if (!customElements.get('quick-order-list')) {
  customElements.define(
    'quick-order-list',
    class QuickOrderList extends BulkAdd {


The biggest change in this class was to handle paginated variants. Primary changes are:

Capturing pagination link clicks and updating content using the section rendering API

Removed assumptions that all variants were present in the quick order list table

There's also some non-trivial refactoring. We were able to simplify a decent amount of code + remove code that wasn't used (example), didn't work (example) / work correctly (example).

lhoffbeck · 2025-01-31T14:00:26Z

assets/quick-order-list.js

+      }
+
+      get cartVariantsForProduct() {
+        return JSON.parse(this.querySelector('[data-cart-contents]')?.innerHTML || '[]');


This is the secret sauce for remove all. Where we used to read variant inputs with non-zero quantities out of the DOM (which doesn't work with paginated variants), we now grab the JSON dump of this cart product (liquid change).

lhoffbeck · 2025-01-31T14:01:14Z

assets/quick-order-list.js

+          event.target.removeEventListener('keydown', handleKeydown);
+        };
+
+        event.target.addEventListener('keydown', handleKeydown);


Bugfix - this event was getting rebound to an input every time it changed 😬

lhoffbeck · 2025-01-31T14:04:29Z

assets/quick-order-list.js

-            });
-        });
-      }
-
      getSectionsToRender() {


Sections / targets got updated to simplify cart update the same as in quick-add-bulk and remove the duplicate section for quick order list.

lhoffbeck · 2025-01-31T14:09:31Z

assets/quick-order-list.js

+          });
+      }
+
+      renderSections(parsedState) {


Primary change here is to how the quickorderlist section is updated to account for pagination. Fixed a few bugs too:

focus state is now preserved after update

no longer ignores cart updates to this product made from other tabs

lhoffbeck · 2025-01-31T14:12:30Z

assets/quick-order-list.js

+          if (this.dataset.action === this.actions.confirm) {
+            this.toggleConfirmation(false, true);
+          } else if (this.dataset.action === this.actions.remove) {
+            const items = this.quickOrderList.cartVariantsForProduct.reduce(


This class was moved down from the start of the file. Only other real change is that we grab the total cart contents from this.quickOrderList.cartVariantsForProduct rather than reading from the DOM.

lhoffbeck · 2025-01-31T14:14:12Z

snippets/quick-order-list.liquid

-    {{ 'icon-error.svg' | inline_asset_content }}
+    <span class="svg-wrapper">
+      {{ 'icon-error.svg' | inline_asset_content }}
+    </span>


Fixes bug where error message would have a gigantic icon

Oliviammarcello

UX works as expected!

ludoboludo

A few things I noticed while testing though they might have been existing bug/issues 🤔

the loading spinner when changing page seem odd to me as it's next to view cart though we're not updating anything about the cart (screenshot)
when I go to remove all, it seem to lose my focus position and sends me back to the top of the product list - recording
some odd scroll behaviour both on the pdp and modal - recording

lhoffbeck · 2025-02-03T14:20:55Z

Thanks @ludoboludo !

the loading spinner when changing page seem odd to me as it's next to view cart though we're not updating anything about the cart (screenshot)

I don't have major feelings about this one way or another. I like the spinner replacing the remove all button when the page is changing to help prevent foot-guns, but we could also handle that by just disabling/enabling that button with the rest of the table. Other thing to call out is that the cart quantity could be updated when changing pages if any quantities changed in another tab.

I'll dig into the other issues, thanks for the videos they helped a ton 🙏

Oliviammarcello · 2025-02-03T15:26:25Z

-the loading spinner when changing page seem odd to me as it's next to view cart though we're not updating anything about the cart (screenshot)

I don't have major feelings about this one way or another. I like the spinner replacing the remove all button when the page is changing to help prevent foot-guns, but we could also handle that by just disabling/enabling that button with the rest of the table. Other thing to call out is that the cart quantity could be updated when changing pages if any quantities changed in another tab.

I agree with your intent @ludoboludo, but with page jumps on load I think keeping the spinner in the "footer" area is probably best to ensure it is always visible.

lhoffbeck · 2025-02-03T15:50:03Z

assets/quick-order-list.js

-      switchVariants(event) {
-        if (event.target.tagName !== 'INPUT') {
-          return;
+      handleScrollIntoView(event) {


I refactored switchVariants into handleScrollIntoView and handleSwitchVariantOnEnter for a few reasons:

the old "scroll into view" behavior didn't work for +/- buttons or tabbing to the pagination links

overly aggressive event binding that could lead to memory leaks

Event binding was arguably the bigger issue here, great example of a memory leak. The first issue is that switchVariants was an event handler for a focusin event on the table and was never unbound, meaning that switchEvents could get bound as many times as the focusin was triggered. The function then bound one of two keydown events to the input that was focused, and never removed these listeners either.

The new code extracts to 2 single-purpose event listeners.

handleSwitchVariantOnEnter is bound to a keydown on the table and determines which input to focus next if the event came from an enter

handleScrollIntoView is bound on keyup, and optionally scrolls the table if the event came from a tab or an enter

lhoffbeck · 2025-02-03T15:50:46Z

snippets/quantity-input.liquid

@@ -11,7 +11,7 @@
 {% endcomment %}

 <quantity-input class="quantity cart-quantity">
-  <button class="quantity__button" name="minus" type="button">
+  <button class="quantity__button" name="minus" type="button" data-target="decrement-{{ variant.id }}">


data-target enables us to refocus inputs after content is updated so keyboard order doesn't get goofed up

lhoffbeck · 2025-02-03T15:55:31Z

@ludoboludo I just pushed up a change that simplified how we do event handling, poking around it seems like it resolved all the funky focus/scroll issues you were running into. Mind taking a look again?

oksanashopify

I tophatted changes and described workflows works as expected for me. Great work 🚀

[questions]

Sorry if it's not related to changes in this PR. When we hit "remove" icon, there is no visual update for customer that quantity is being removed and customer just can continue to hit "remove" button. Video. Shout it have disabled or maybe loading state?

lhoffbeck · 2025-02-03T18:02:13Z

Sorry if it's not related to changes in this PR. When we hit "remove" icon, there is no visual update for customer that quantity is being removed and customer just can continue to hit "remove" button. Video. Shout it have disabled or maybe loading state?

Yeah agreed, that's pretty gross. I'll flag it with the team that owns the component to see if they want to do some UX exploration 👍

LA1CH3

Lacking storefront context but based off PR description LGTM!

Only thing I am unsure about whether its expected or not is that for a product without paginated variants, there is no pagination (as expected) but also no "Total items", "Product subtotal", "View cart" and "Remove all"- is that expected?

single.variant.vs.multi.variant.sf.mp4

LA1CH3 · 2025-02-06T18:06:15Z

assets/quick-add-bulk.js

@@ -55,15 +53,15 @@ if (!customElements.get('quick-add-bulk')) {

      listenForActiveInput() {
        if (!this.classList.contains('hidden')) {
-          this.getInput().addEventListener('focusin', (event) => event.target.select());
+          this.getInput()?.addEventListener('focusin', (event) => event.target.select());


Nit but to remove some repetition in a few of these areas, you could do something like:

// or just rename `getInput` as a getter get input() { return this.getInput(); } listenForActiveInput() { if (!this.classList.contains('hidden') && this.input) { this.input.addEventListener(...); } } // same thing for `listenForKeydown`

LA1CH3 · 2025-02-06T18:14:11Z

assets/quick-order-list.js

-          this.totalBarPosition = window.innerHeight - this.getTotalBar().offsetHeight;
+        this.totalBar = this.getTotalBar();
+        if (this.totalBar) {
+          this.totalBarPosition = window.innerHeight - this.totalBar.offsetHeight;

          window.addEventListener('resize', () => {


Pre-existing issue, but we should probably return the event listener here so we can unsubscribe from the resize event when the quick order list is unmounted/detached.

LA1CH3 · 2025-02-06T18:15:45Z

assets/quick-order-list.js

+        this.allInputsArray = Array.from(this.querySelectorAll('input[type="number"]'));
+
+        this.querySelectorAll('quantity-input').forEach((qty) => {
+          const debouncedOnChange = debounce(this.onChange.bind(this), 250);


Nit- can we store this debounce time value as a constant somewhere?

LA1CH3 · 2025-02-06T18:19:20Z

assets/quick-order-list.js

I realize this is all pre-existing, but there's a huge amount of selectors just hard-coded and repeated throughout this file- would be great to store them all as constants/variables and just reference those.

Yeah, totally agree. I was trying to keep this PR as tight as possible to avoid introducing regressions.

assets/quick-order-list.js

ludoboludo

Not a thorough review but from what I've checked, it didn't seem to introduce regressions.
A bit harder to test when not in production 😅

lhoffbeck · 2025-02-13T19:10:22Z

Lacking storefront context but based off PR description LGTM!

Only thing I am unsure about whether its expected or not is that for a product without paginated variants, there is no pagination (as expected) but also no "Total items", "Product subtotal", "View cart" and "Remove all"- is that expected?

single.variant.vs.multi.variant.sf.mp4

Yeah good callout, this is existing behavior. My assumption is that the original devs built it like this because there's very little (no?) customer benefit to showing a quick order list with a single variant vs the buyer just using the main product form.

add keyboard and mouse click differentiation for autofocus shift. Update 1 translation file Update 1 translation file Update 2 translation files Update 1 translation file Update 1 translation file Migrate to using sections with page param Fix bug where refresh was broken bugfix: don't clear loading state when there are still pending reqeusts Fix pagenumber and focus state to work with renderSections Cleanup Fix quick add bulk bugs Update 1 translation file Update 3 translation files Fix bugs when pagination links are not displayed or quick order list total is not shown Bugfixes: don't scroll on input refocus, scroll top after changing page Bugfix -- fix duplicated event listener binding + scroll on tab Bugfix -- scroll container when in pop up view Update 3 translation files Update 2 translation files Back out changes to quantity spinner Minor cleanup Update 2 translation files Update 1 translation file Update 1 translation file Migrate to delegated event listeners to simplify and fix focus bugs Update content type for cart json PR feedback Co-authored-by: Parsa Hemmati <parsa.hemmati@shopify.com>

github-actions bot added the cla-needed label Jan 28, 2025

lhoffbeck force-pushed the update-quick-order-list-to-use-paginated-variants branch from 92127ee to 5bff9dc Compare January 28, 2025 15:23

github-actions bot removed the cla-needed label Jan 28, 2025

lhoffbeck commented Jan 31, 2025

View reviewed changes

lhoffbeck requested review from tyleralsbury, maxariss, oksanashopify and ludoboludo January 31, 2025 14:18

Oliviammarcello approved these changes Jan 31, 2025

View reviewed changes

ludoboludo reviewed Jan 31, 2025

View reviewed changes

lhoffbeck force-pushed the update-quick-order-list-to-use-paginated-variants branch from d0ee95a to 9a7b42d Compare February 3, 2025 15:23

lhoffbeck commented Feb 3, 2025

View reviewed changes

lhoffbeck requested a review from ludoboludo February 3, 2025 15:55

oksanashopify approved these changes Feb 3, 2025

View reviewed changes

lhoffbeck added the Next Release Candidate label Feb 3, 2025

LA1CH3 reviewed Feb 6, 2025

View reviewed changes

ludoboludo approved these changes Feb 7, 2025

View reviewed changes

lhoffbeck force-pushed the update-quick-order-list-to-use-paginated-variants branch from 70f65ee to 770517c Compare February 13, 2025 21:14

lhoffbeck force-pushed the update-quick-order-list-to-use-paginated-variants branch from 770517c to 47c315e Compare February 24, 2025 13:54

lhoffbeck merged commit c661a79 into main Feb 24, 2025
8 checks passed

lhoffbeck deleted the update-quick-order-list-to-use-paginated-variants branch February 24, 2025 14:17

lhoffbeck mentioned this pull request Feb 25, 2025

Update quick order list to not override table until all enqueued requests are processed #3729

Merged

9 tasks

		const quickBulkElement = this.closest('quick-order-list') \|\| this.closest('quick-add-bulk');
		quickBulkElement.updateMultipleQty(items);

Update quick order list to use paginated product.variants #3710

Update quick order list to use paginated product.variants #3710

Conversation

lhoffbeck commented Jan 28, 2025 • edited Loading

PR Summary:

Why are these changes introduced?

What approach did you take?

Testing steps/scenarios

Demo links

Checklist

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhoffbeck Jan 31, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oliviammarcello left a comment

Choose a reason for hiding this comment

ludoboludo left a comment

Choose a reason for hiding this comment

lhoffbeck commented Feb 3, 2025

Oliviammarcello commented Feb 3, 2025

lhoffbeck Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhoffbeck commented Feb 3, 2025

oksanashopify left a comment

Choose a reason for hiding this comment

lhoffbeck commented Feb 3, 2025

LA1CH3 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

ludoboludo left a comment

Choose a reason for hiding this comment

lhoffbeck commented Feb 13, 2025

lhoffbeck commented Jan 28, 2025 •

edited

Loading

lhoffbeck Jan 31, 2025 •

edited

Loading

lhoffbeck Feb 3, 2025 •

edited

Loading