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

feature(storefront): BCTHEME-68 Add tooltips for carousel "Previous" and "Next" buttons #1749

Merged
merged 2 commits into from
Aug 26, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
14 changes: 10 additions & 4 deletions assets/js/test-unit/theme/carousel.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ describe('carousel', () => {
var slickSpy = spyOn(multipleSlidesElement, 'slick');
carousel();
expect(slickSpy).toHaveBeenCalledWith({
dots: true,
customPaging: expect.any(Function)
accessibility: false,
arrows: true,
customPaging: expect.any(Function),
dots: true,
});
});

Expand All @@ -54,7 +56,11 @@ describe('carousel', () => {
spyOn(jQuery.fn, 'find').and.returnValue(multipleSlidesElement);
var slickSpy = spyOn(multipleSlidesElement, 'slick');
carousel();
expect(slickSpy).toHaveBeenCalledWith({ dots: false });
expect(slickSpy).toHaveBeenCalledWith({
accessibility: false,
arrows: false,
customPaging: expect.any(Function),
dots: false,
});
});
});

70 changes: 51 additions & 19 deletions assets/js/theme/common/carousel.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import 'slick-carousel';

const integerRegExp = /[0-9]+/;

const setSlideTabindexes = ($slides) => {
$slides.each((index, element) => {
const $element = $(element);
Expand All @@ -14,38 +16,68 @@ const showCarouselIfSlidesAnalizedSetup = ($carousel) => {
analizedSlides.push($slide);
if ($slides.length === analizedSlides.length) {
$carousel.addClass('is-visible');
setSlideTabindexes($('.slick-slide'));
}
};
};

const arrowAriaLabling = ($arrowLeft, $arrowRight, currentSlide, lastSlide) => {
if (lastSlide < 2) return;
if ($arrowLeft.length === 0 || $arrowRight.length === 0) return;
Comment on lines +24 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we concatenate conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but in this case I would like to have different conditions, because it is more obvious what cases are unsuitable for continuing function


const arrowAriaLabelBaseText = $arrowLeft.attr('aria-label');

const isInit = arrowAriaLabelBaseText.includes(['NUMBER']);
BC-tymurbiedukhin marked this conversation as resolved.
Show resolved Hide resolved
const valueToReplace = isInit ? '[NUMBER]' : integerRegExp;

const leftGoToNumber = currentSlide === 1 ? lastSlide : currentSlide - 1;
const arrowLeftText = arrowAriaLabelBaseText.replace(valueToReplace, leftGoToNumber);
$arrowLeft.attr('aria-label', arrowLeftText);

const rightGoToNumber = currentSlide === lastSlide ? 1 : currentSlide + 1;
const arrowRightText = arrowAriaLabelBaseText.replace(valueToReplace, `${rightGoToNumber}`);
$arrowRight.attr('aria-label', arrowRightText);
};

const onCarouselChange = (event, carousel) => {
const { options: { prevArrow, nextArrow }, currentSlide, slideCount } = carousel;

setSlideTabindexes($(event.target).find('.slick-slide'));

arrowAriaLabling($(prevArrow), $(nextArrow), currentSlide + 1, slideCount);
BC-tymurbiedukhin marked this conversation as resolved.
Show resolved Hide resolved
};

export default function () {
const $carousel = $('[data-slick]');
const $carouselCollection = $('[data-slick]');

if ($carouselCollection.length === 0) return;

if ($carousel.length === 0) return;
$carouselCollection.on('init', onCarouselChange);
$carouselCollection.on('afterChange', onCarouselChange);

const isMultipleSlides = $carousel[0].childElementCount > 1;
$carouselCollection.each((index, carousel) => {
// getting element using find to pass jest test
const $carousel = $(document).find(carousel);

const slickSettingsObj = isMultipleSlides
? {
dots: true,
customPaging: () => (
const isMultipleSlides = $carousel.children().length > 1;
const customPaging = isMultipleSlides
? () => (
'<button type="button"></button>'
),
}
: {
dots: false,
)
: () => {};

const options = {
accessibility: false,
arrows: isMultipleSlides,
customPaging,
dots: isMultipleSlides,
};

$carousel.slick(slickSettingsObj);

$carousel.on('afterChange', () => {
setSlideTabindexes($('.slick-slide'));
$carousel.slick(options);
});

const $slidesNodes = $('.heroCarousel-slide');

const showCarouselIfSlidesAnalized = showCarouselIfSlidesAnalizedSetup($carousel)($slidesNodes);
const $heroCarousel = $carouselCollection.filter('.heroCarousel');
const $slidesNodes = $heroCarousel.find('.heroCarousel-slide');
const showCarouselIfSlidesAnalized = showCarouselIfSlidesAnalizedSetup($heroCarousel)($slidesNodes);

$slidesNodes.each((index, element) => {
const $element = $(element);
Expand Down
3 changes: 3 additions & 0 deletions lang/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -880,5 +880,8 @@
"maintenance_showstore_link": "Click here to see what your visitors will see.",
"prelaunch_header": "Your storefront is private. Share your site with preview code:",
"page_builder_link": "Design this page in Page Builder"
},
"carousel": {
"arrowAriaLabel": "Go to slide [NUMBER] of"
}
}
17 changes: 14 additions & 3 deletions templates/components/carousel.html
Original file line number Diff line number Diff line change
@@ -1,16 +1,22 @@
<section class="heroCarousel"
data-slick='{
"arrows": {{arrows}},
"mobileFirst": true,
"slidesToShow": 1,
"slidesToScroll": 1,
"autoplay": true,
"autoplaySpeed": {{carousel.swap_frequency}},
"lazyLoad": "anticipated",
"accessibility": false
BC-tymurbiedukhin marked this conversation as resolved.
Show resolved Hide resolved
"slide": ".hero-slide",
BC-tymurbiedukhin marked this conversation as resolved.
Show resolved Hide resolved
"prevArrow": ".js-hero-prev-arrow",
"nextArrow": ".js-hero-next-arrow"
}'>
{{#if arrows}}
{{#if carousel.slides.length '>' 1}}
BC-tymurbiedukhin marked this conversation as resolved.
Show resolved Hide resolved
<button aria-label="{{lang 'carousel.arrowAriaLabel'}} {{carousel.slides.length}}" class="js-hero-prev-arrow slick-prev slick-arrow">hero-prev-arrow</button>
{{/if}}
{{/if}}
{{#each carousel.slides}}
<a href="{{url}}">
<a class="hero-slide" href="{{url}}">
<div class="heroCarousel-slide {{#if ../theme_settings.homepage_stretch_carousel_images}}stretch{{/if}} {{#if @first}}heroCarousel-slide--first{{/if}}">
<div class="heroCarousel-image-wrapper">
{{#if @first}}
Expand Down Expand Up @@ -43,4 +49,9 @@
</div>
</a>
{{/each}}
{{#if arrows}}
{{#if carousel.slides.length '>' 1}}
BC-tymurbiedukhin marked this conversation as resolved.
Show resolved Hide resolved
<button aria-label="{{lang 'carousel.arrowAriaLabel'}} {{carousel.slides.length}}" class="js-hero-next-arrow slick-next slick-arrow">hero-next-arrow</button>
{{/if}}
{{/if}}
</section>
21 changes: 14 additions & 7 deletions templates/components/products/carousel.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,31 +2,38 @@
data-list-name="{{list}}"
data-slick='{
"dots": true,
"infinite": false,
"mobileFirst": true,
"slidesToShow": 2,
"slidesToScroll": 2,
"slidesToScroll": 1,
BC-tymurbiedukhin marked this conversation as resolved.
Show resolved Hide resolved
"slide": ".product-slide",
BC-tymurbiedukhin marked this conversation as resolved.
Show resolved Hide resolved
"prevArrow": ".js-product-prev-arrow",
"nextArrow": ".js-product-next-arrow",
"responsive": [
{
"breakpoint": 800,
"settings": {
"slidesToShow": {{ columns }},
"slidesToScroll": 3
"slidesToScroll": 1
}
},
{
"breakpoint": 550,
"settings": {
"slidesToShow": 3,
"slidesToScroll": 3
"slidesToScroll": 1
BC-tymurbiedukhin marked this conversation as resolved.
Show resolved Hide resolved
}
}
]
}'
>
}'>
{{#if products.length '>' 1}}
<button aria-label="{{lang 'carousel.arrowAriaLabel'}} {{products.length}}" class="js-product-prev-arrow slick-prev slick-arrow"></button>
{{/if}}
{{#each products}}
<div class="productCarousel-slide">
<div class="productCarousel-slide product-slide">
{{> components/products/card settings=../settings theme_settings=../theme_settings customer=../customer event="list" position=(add @index 1)}}
</div>
{{/each}}
{{#if products.length '>' 1}}
<button aria-label="{{lang 'carousel.arrowAriaLabel'}} {{products.length}}" class="js-product-next-arrow slick-next slick-arrow"></button>
{{/if}}
</section>