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

Follow ups for accessibility of announcements slider #2580

Merged
merged 8 commits into from
May 2, 2023
Merged

Follow ups for accessibility of announcements slider #2580

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1727d86
Stop auto-rotation after first interaction with an arrow button.
eugenekasimov Apr 27, 2023
4dcf680
Change 'resize' to 'change' and max-width to min-width.
eugenekasimov Apr 27, 2023
a89de56
Apply the current changes to the announcement bar only.
eugenekasimov Apr 27, 2023
9396596
Renaming variables in JS.
eugenekasimov Apr 28, 2023
00d23e0
Add a condition for the announcement bar for JS.
eugenekasimov Apr 28, 2023
441ab20
Refactoring JS code.
eugenekasimov Apr 28, 2023
adefa39
Refactor JS.
eugenekasimov May 1, 2023
a92da33
Combine mediaQuery into array.
eugenekasimov May 2, 2023
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
18 changes: 16 additions & 2 deletions assets/global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,7 @@ class SlideshowComponent extends SliderComponent {
super();
this.sliderControlWrapper = this.querySelector('.slider-buttons');
this.enableSliderLooping = true;
this.arrowButtonWasInteracted = false;
eugenekasimov marked this conversation as resolved.
Show resolved Hide resolved
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved

if (!this.sliderControlWrapper) return;

Expand All @@ -712,11 +713,17 @@ class SlideshowComponent extends SliderComponent {
this.slider.addEventListener('scroll', this.setSlideVisibility.bind(this));
this.setSlideVisibility();

this.desktopLayout = window.matchMedia('(min-width: 750px)');
this.reducedMotion = window.matchMedia('(prefers-reduced-motion: reduce)');

this.reducedMotion.addEventListener('change', () => {
if (this.slider.getAttribute('data-autoplay') === 'true') this.setAutoPlay();
});

this.desktopLayout.addEventListener('change', (event) => {
if (this.slider.getAttribute('data-autoplay') === 'true') this.setAutoPlay();
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
});

if (this.slider.getAttribute('data-autoplay') === 'true') this.setAutoPlay();
}

Expand All @@ -727,13 +734,20 @@ class SlideshowComponent extends SliderComponent {
this.addEventListener('focusin', this.focusInHandling.bind(this));
this.addEventListener('focusout', this.focusOutHandling.bind(this));

this.sliderArrowButtons = this.querySelectorAll('.announcement-bar-slider .slider-button');
eugenekasimov marked this conversation as resolved.
Show resolved Hide resolved
this.sliderArrowButtons.forEach((button) => {
button.addEventListener('click', () => {
this.arrowButtonWasInteracted = true;
}, {once: true});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I don't think you need to add/create a new event listener. We already have an existing listener on the buttons from the SliderComponent.
So you could just add the functionality that you need in the onButtonClick function specifically when it's the announcement bar buttons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Video explanation.

I know you use the once: true on the listener you declared but you should be able to run your specific logic just once in onButtonClick as well by have a check.

Though because it's something that isn't general to slideshows but only in one specific use case (announcement bars), it might be better to keep it separate 🤔

@stufreen do you have a preference yourself ?


if (this.querySelector('.slideshow__autoplay')) {
this.sliderAutoplayButton = this.querySelector('.slideshow__autoplay');
this.sliderAutoplayButton.addEventListener('click', this.autoPlayToggle.bind(this));
this.autoplayButtonIsSetToPlay = true;
this.play();
} else {
this.reducedMotion.matches ? this.pause() : this.play();
this.reducedMotion.matches || this.arrowButtonWasInteracted || !this.desktopLayout.matches ? this.pause() : this.play();
}
}

Expand Down Expand Up @@ -782,7 +796,7 @@ class SlideshowComponent extends SliderComponent {
event.target === this.sliderAutoplayButton || this.sliderAutoplayButton.contains(event.target);
if (!this.autoplayButtonIsSetToPlay || focusedOnAutoplayButton) return;
this.play();
} else if (!this.reducedMotion.matches) {
} else if (!this.reducedMotion.matches && !this.arrowButtonWasInteracted && this.desktopLayout.matches) {
this.play();
}
}
Expand Down
6 changes: 3 additions & 3 deletions sections/announcement-bar.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,9 @@
{
"type": "range",
"id": "change_slides_speed",
"min": 3,
"max": 9,
"step": 2,
"min": 5,
"max": 10,
"step": 1,
"unit": "s",
"label": "t:sections.announcement-bar.settings.change_slides_speed.label",
"default": 5
Expand Down