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 query selectors to fix selection issue #3730

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

lhoffbeck
Copy link
Contributor

PR Summary:

Fixes bulk quick order component bug where invalid ID prevented cart subscription handler from working

Why are these changes introduced?

This PR from last April tweaked which html element cached the section.id in a data attribute, but missed some query selectors in the corresponding JS.

The impact of this is that if there are multiple quick order bulk widgets present on a collection page, the component isn't able to read the section ID from the dom and (ultimately) a network call 404s. Demo video.

Credit to @stanley-xu for finding this issue.

What approach did you take?

Updated the selector to point to the correct location in the DOM.

Demo links

https://6483d52vk55uzev9-58156712071.shopifypreview.com

Checklist

Copy link

@stanley-xu stanley-xu left a comment

Choose a reason for hiding this comment

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

LGTM!

onCartUpdate() {
return new Promise((resolve, reject) => {
fetch(`${this.getSectionsUrl()}?section_id=${this.closest('.collection').dataset.id}`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was trying to find data-id on the .collection div, which doesn't have that attribute

.then((response) => response.text())
.then((responseText) => {
const html = new DOMParser().parseFromString(responseText, 'text/html');
const sourceQty = html.querySelector(
`#quick-add-bulk-${this.dataset.id}-${this.closest('.collection').dataset.id}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.dataset.id doesn't exist, updated to be this.dataset.index like other accessors in this file

@lhoffbeck lhoffbeck merged commit 75b5a8a into main Feb 25, 2025
8 checks passed
@lhoffbeck lhoffbeck deleted the lh-fix-quick-order-list-bulk-query-selectors branch February 25, 2025 23:26
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.

3 participants