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

Move slideshow control buttons when set to autorotate #1523

Merged
merged 2 commits into from
Mar 29, 2022
Merged

Move slideshow control buttons when set to autorotate #1523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
780877f
Move slideshow control buttons when set to autorotate
ludoboludo Mar 22, 2022
1c70c25
edit class name
ludoboludo Mar 29, 2022
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
10 changes: 8 additions & 2 deletions assets/component-slideshow.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
slideshow-component {
position: relative;
display: block;
display: flex;
flex-direction: column;
}

@media screen and (max-width: 989px) {
Expand Down Expand Up @@ -108,8 +109,13 @@ slideshow-component .slideshow.banner {
border: 0.1rem solid rgba(var(--color-foreground), 0.08);
}

.slideshow__controls--top {
order: 2;
z-index: 1;
}

@media screen and (max-width: 749px) {
slideshow-component.page-width .slideshow.banner--mobile-bottom + .slideshow__controls {
.slideshow__controls--border-radius-mobile {
border-bottom-right-radius: var(--text-boxes-radius);
border-bottom-left-radius: var(--text-boxes-radius);
}
Expand Down
47 changes: 43 additions & 4 deletions sections/slideshow.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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-radius-mobile{% 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' -%}
<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"
Expand Down Expand Up @@ -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 -%}
Copy link
Contributor

Choose a reason for hiding this comment

The 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 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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. 🤷

Copy link
Contributor

@kmeleta kmeleta Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The 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.

The duplicate isn't that great but also not really a problem I think except for more code to read.

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-radius-mobile{% 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' -%}
Expand All @@ -121,7 +162,6 @@
</button>
{%- endif -%}
</div>

<noscript>
<div class="slider-buttons">
<div class="slider-counter">
Expand All @@ -134,7 +174,6 @@
</div>
</noscript>
{%- endif -%}

</slideshow-component>

{%- if request.design_mode -%}
Expand Down