-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Move slideshow control buttons when set to autorotate #1523
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,47 @@ | |
{%- endif -%} | ||
|
||
<slideshow-component class="slider-mobile-gutter{% if section.settings.layout == 'grid' %} page-width{% endif %}{% if section.settings.show_text_below %} mobile-text-below{% endif %}" role="region" aria-roledescription="{{ 'sections.slideshow.carousel' | t }}" aria-label="{{ section.settings.accessibility_info | escape }}"> | ||
{%- if section.settings.auto_rotate and section.blocks.size > 1 -%} | ||
<div class="slideshow__controls slideshow__controls--top slider-buttons no-js-hidden{% if section.settings.show_text_below %} slideshow__controls--border{% endif %}"> | ||
ludoboludo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<button type="button" class="slider-button slider-button--prev" name="previous" aria-label="{{ 'sections.slideshow.previous_slideshow' | t }}" aria-controls="Slider-{{ section.id }}">{% render 'icon-caret' %}</button> | ||
<div class="slider-counter slider-counter--{{ section.settings.slider_visual }}{% if section.settings.slider_visual == 'counter' or section.settings.slider_visual == 'numbers' %} caption{% endif %}"> | ||
{%- if section.settings.slider_visual == 'counter' -%} | ||
<span class="slider-counter--current">1</span> | ||
<span aria-hidden="true"> / </span> | ||
<span class="visually-hidden">{{ 'general.slider.of' | t }}</span> | ||
<span class="slider-counter--total">{{ section.blocks.size }}</span> | ||
{%- else-%} | ||
<div class="slideshow__control-wrapper"> | ||
{%- for block in section.blocks -%} | ||
<button class="slider-counter__link slider-counter__link--{{ section.settings.slider_visual }} link" aria-label="{{ 'sections.slideshow.load_slide' | t }} {{ forloop.index }} {{ 'general.slider.of' | t }} {{ forloop.length }}" aria-controls="Slider-{{ section.id }}"> | ||
{%- if section.settings.slider_visual == 'numbers' -%}{{ forloop.index }}{% else %}<span class="dot"></span>{%- endif -%} | ||
</button> | ||
{%- endfor -%} | ||
</div> | ||
{%- endif -%} | ||
</div> | ||
<button type="button" class="slider-button slider-button--next" name="next" aria-label="{{ 'sections.slideshow.next_slideshow' | t }}" aria-controls="Slider-{{ section.id }}">{% render 'icon-caret' %}</button> | ||
|
||
{%- if section.settings.auto_rotate -%} | ||
<button type="button" class="slideshow__autoplay slider-button no-js-hidden{% if section.settings.auto_rotate == false %} slideshow__autoplay--paused{% endif %}" aria-label="{{ 'sections.slideshow.pause_slideshow' | t }}"> | ||
{%- render 'icon-pause' -%} | ||
{%- render 'icon-play' -%} | ||
</button> | ||
{%- endif -%} | ||
</div> | ||
<noscript> | ||
<div class="slider-buttons"> | ||
<div class="slider-counter"> | ||
{%- for block in section.blocks -%} | ||
<a href="#Slide-{{ section.id }}-{{ forloop.index }}" class="slider-counter__link link" aria-label="{{ 'sections.slideshow.load_slide' | t }} {{ forloop.index }} {{ 'general.slider.of' | t }} {{ forloop.length }}"> | ||
{{ forloop.index }} | ||
</a> | ||
{%- endfor -%} | ||
</div> | ||
</div> | ||
</noscript> | ||
{%- endif -%} | ||
|
||
<div class="slideshow banner banner--{{ section.settings.slide_height }} grid grid--1-col slider slider--everywhere{% if section.settings.show_text_below %} banner--mobile-bottom{% endif %}{% if section.blocks.first.settings.image == blank %} slideshow--placeholder{% endif %}" | ||
id="Slider-{{ section.id }}" | ||
aria-live="polite" | ||
|
@@ -93,8 +134,8 @@ | |
{%- endfor -%} | ||
</div> | ||
|
||
{%- if section.blocks.size > 1 -%} | ||
<div class="slideshow__controls slider-buttons no-js-hidden"> | ||
{%- if section.blocks.size > 1 and section.settings.auto_rotate == false -%} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Obviously messing with the tab order isn't ideal and we should only do it with good reason, but I could probably be sold on keeping the controls-first order regardless of the autorotate setting honestly. It's edge-casey, but if a shop has multiple sliders and some of them auto rotate and some don't, the KO experience would vary between these instances. Just a thought 🤷 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm yeah that means tab order is going to be a bit odd when not set to autoplay. Honestly whatever @svinkle thinks is best. The duplicate isn't that great but also not really a problem I think except for more code to read. 🤷 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah absolutely. I saw Scott's recommendation in the issue was to only mess with the tab order for autorotate and that's fine with me if that's still the case, just wanted to add another consideration to that discussion.
Yeah it's fine. We already have to this in plenty of places I think. If there was a way to cache markup in an assign or something, I'd want to do that so there's a single source of truth for the code. I worry about changes to markup being made in one instance and not the other when we have to do this. It's also very difficult to tell at a glance if there are any purposeful differences between each instance. |
||
<div class="slideshow__controls slider-buttons no-js-hidden{% if section.settings.show_text_below %} slideshow__controls--border{% endif %}"> | ||
<button type="button" class="slider-button slider-button--prev" name="previous" aria-label="{{ 'sections.slideshow.previous_slideshow' | t }}" aria-controls="Slider-{{ section.id }}">{% render 'icon-caret' %}</button> | ||
<div class="slider-counter slider-counter--{{ section.settings.slider_visual }}{% if section.settings.slider_visual == 'counter' or section.settings.slider_visual == 'numbers' %} caption{% endif %}"> | ||
{%- if section.settings.slider_visual == 'counter' -%} | ||
|
@@ -121,7 +162,6 @@ | |
</button> | ||
{%- endif -%} | ||
</div> | ||
|
||
<noscript> | ||
<div class="slider-buttons"> | ||
<div class="slider-counter"> | ||
|
@@ -134,7 +174,6 @@ | |
</div> | ||
</noscript> | ||
{%- endif -%} | ||
|
||
</slideshow-component> | ||
|
||
{%- if request.design_mode -%} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a classname with "radius" and/or "mobile" in it would be a little more accurate? Unless there are additional/future considerations here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I went with the longish
slideshow__controls--border-radius-mobile
🙂