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.

Sign up for GitHub

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

Jump to bottom

Prevent queued UI changes from variant selections #2030

Merged
merged 1 commit into from
Oct 18, 2022
Merged

Prevent queued UI changes from variant selections #2030

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions assets/global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,10 +840,14 @@ class VariantSelects extends HTMLElement {
}

renderProductInfo() {
const requestedVariantId = this.currentVariant.id;
const activeElementId = document.activeElement.id;
fetch(`${this.dataset.url}?variant=${this.currentVariant.id}&section_id=${this.dataset.originalSection ? this.dataset.originalSection : this.dataset.section}`)
fetch(`${this.dataset.url}?variant=${requestedVariantId}&section_id=${this.dataset.originalSection ? this.dataset.originalSection : this.dataset.section}`)
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Leaving this as a comment so we can thread discussion.

From a UX perspective, I still see some downsides with this fetch happening while the user is interacting, though.

If you're using the Dropdown UI, the list of options will auto-close when the variant list is updated:

18-09-dxtqw-yzdyg.mp4

If you're using the Pills UI with a keyboard, the UI won't update until you stop interacting with the component. This means that some variants will be announced as "unavailable" for screen readers despite being available, because the UI is still reflecting the previous state:

18-11-ykiaf-gkiwm.mp4

At this point, I'm not sure if we should reconsider our approach to fetch & re-insert the UI and move this logic of disabling unavailable variants directly to JS so it's quicker. This component needs faster results than the price, as it's the component that triggers the fetch action in the first place and the one the keyboard focus lives in while the fetch is happening. An alternative would be to disable the UI and display a loading indicator, but I'm not sure what's the ideal a11y flow for this.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per our discussions, the approach mentioned fits well into what Ludo is already working on here #2031 but this PR still provides some improvements and should be merged.

.then((response) => response.text())
.then((responseText) => {
// prevent unnecessary ui changes from abandoned selections
if (this.currentVariant.id !== requestedVariantId) return;

const html = new DOMParser().parseFromString(responseText, 'text/html')
const destination = document.getElementById(`price-${this.dataset.section}`);
const source = html.getElementById(`price-${this.dataset.originalSection ? this.dataset.originalSection : this.dataset.section}`);
Expand All @@ -857,7 +861,7 @@ class VariantSelects extends HTMLElement {

if (price) price.classList.remove('visibility-hidden');
this.toggleAddButton(!this.currentVariant.available, window.variantStrings.soldOut);

document.querySelector('variant-radios') ? this.querySelector(`[for="${activeElementId}"]`).focus() : this.querySelector(`#${activeElementId}`).focus();
});
}
Expand Down