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

Update slider shadow padding implementation #1292

Merged
merged 4 commits into from
Feb 3, 2022
Merged

Update slider shadow padding implementation #1292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
35fbb7a
Update slider shadow padding implementation
kmeleta Jan 31, 2022
9fbc384
Adjust blog isolate
kmeleta Feb 1, 2022
2af6578
bem
kmeleta Feb 1, 2022
d7f333e
bem adjust
kmeleta Feb 1, 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
8 changes: 3 additions & 5 deletions assets/base.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,7 @@
--color-icon: rgb(var(--color-base-outline-button-labels));
}

.product-grid,
.collection-list,
.blog__posts,
.contains-card,
.card {
--border-radius: var(--card-corner-radius);
--border-width: var(--card-border-width);
Expand All @@ -168,8 +166,8 @@
--shadow-opacity: var(--card-shadow-opacity);
}

.multicolumn-list,
.multicolumn-card {
.contains-content-container,
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
.content-container {
--border-radius: var(--text-boxes-radius);
--border-width: var(--text-boxes-border-width);
--border-opacity: var(--text-boxes-border-opacity);
Expand Down
32 changes: 24 additions & 8 deletions assets/component-slider.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,12 @@ slider-component {
}

.slider__slide {
--focus-outline-padding: 0.5rem;
--shadow-padding-top: calc(var(--shadow-vertical-offset) * -1 + var(--shadow-blur-radius));
--shadow-padding-bottom: calc(var(--shadow-vertical-offset) + var(--shadow-blur-radius));
scroll-snap-align: start;
flex-shrink: 0;
padding-bottom: 0;
}

@media screen and (max-width: 749px) {
Expand All @@ -24,13 +28,20 @@ slider-component {
scroll-padding-left: 1.5rem;
-webkit-overflow-scrolling: touch;
margin-bottom: 1rem;
padding-top: max(0.5rem, calc(var(--shadow-vertical-offset) * -1 + var(--shadow-blur-radius)));
padding-bottom: max(0.5rem, calc(var(--shadow-vertical-offset) + var(--shadow-blur-radius)));
}

.slider.slider--mobile .slider__slide {
margin-bottom: 0;
padding-bottom: 0;
padding-top: max(var(--focus-outline-padding), var(--shadow-padding-top));
padding-bottom: max(var(--focus-outline-padding), var(--shadow-padding-bottom));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also have a padding-top/bottom value when it's not a slider, either on the ul or li:last-child/li:first-child

Copy link
Contributor

Choose a reason for hiding this comment

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

Or let people use the section padding setting to deal with it 🤷

Copy link
Contributor Author

@kmeleta kmeleta Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think my initial thought was using section padding. In a perfect world where the section shadow/z-index thing is fixed, that doesn't even get cut off by the section below it. This PR is only needed because of the overflow: hidden on sliders.

Copy link
Contributor

Choose a reason for hiding this comment

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

Curious to learn the settings combination as I wasn't able to get it on Collection cards, but mainly on Multicolumn.

Copy link
Contributor Author

@kmeleta kmeleta Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That's kind of interesting Meli that not all sections do that. There may be a discrepancy between the isolate implementations in those sections. In any case, it appears it's an adjacent section cutting the shadow off, not an overflow: hidden from sliders. So I'd probably rather address the actual problem rather than use the magic padding here as a bandaid.

Copy link
Contributor Author

@kmeleta kmeleta Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I just pushed a change for that. I don't want to go down a rabbit hole in this PR, but if that looks better you, we'll roll with it. Otherwise I'll revert and address separately. I think there's a issue open for avoiding the isolate scenario altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanted to drop the issue referenced above about potentially fixing this by removing the isolate approach here for reference #1241

}

.slider.slider--mobile.contains-card--standard .slider__slide:not(.collection-list__item--no-media) {
padding-bottom: var(--focus-outline-padding);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should use the max(... here as well so the shadow doesn't cover the product title:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mention in the PR desc that it's something we can consider, but could also be solved by allowing spacing settings for card titles or something. I don't really like the idea of handling this the same way. I feel the gap between image and title could end up being unnecessarily far apart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If UX likes the idea of just using the same automatic padding here, I'll add it, but if it's something to be discussed more, it should be handled separately. cc @melissaperreault

Copy link
Contributor

@melissaperreault melissaperreault Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Since merchants have the ability to set the opacity to maintain good contrast, I would handle separately please because we need to explore, define and observe how this is being used. We'll consider how it affects section headings as well when the vertical offset is negative for both desktop and mobile.

There might be more opportunity to scale systematically dependent of the shadow opacity, offset, and blur for instance or potentially add new settings.

}

.slider.slider--mobile.contains-content-container .slider__slide {
--focus-outline-padding: 0rem;
}
}

Expand All @@ -47,7 +58,6 @@ slider-component {

.slider.slider--tablet-up .slider__slide {
margin-bottom: 0;
padding-bottom: 0;
}
}

Expand All @@ -61,13 +71,20 @@ slider-component {
scroll-padding-left: 1.5rem;
-webkit-overflow-scrolling: touch;
margin-bottom: 1rem;
padding-top: max(0.5rem, calc(var(--shadow-vertical-offset) * -1 + var(--shadow-blur-radius)));
padding-bottom: max(0.5rem, calc(var(--shadow-vertical-offset) + var(--shadow-blur-radius)));
}

.slider.slider--tablet .slider__slide {
margin-bottom: 0;
padding-bottom: 0;
padding-top: max(var(--focus-outline-padding), var(--shadow-padding-top));
padding-bottom: max(var(--focus-outline-padding), var(--shadow-padding-bottom));
}

.slider.slider--tablet.contains-card--standard .slider__slide:not(.collection-list__item--no-media) {
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
padding-bottom: var(--focus-outline-padding);
}

.slider.slider--tablet.contains-content-container .slider__slide {
--focus-outline-padding: 0rem;
}
}

Expand All @@ -89,7 +106,6 @@ slider-component {

.slider.slider--everywhere .slider__slide {
margin-bottom: 0;
padding-bottom: 0;
scroll-snap-align: center;
}

Expand Down
4 changes: 2 additions & 2 deletions sections/collection-list.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
{% endunless %}

<slider-component class="slider-mobile-gutter">
<ul class="collection-list grid grid--1-col{% if section.blocks.size < 5 %} grid--{{ section.blocks.size }}-col-tablet{% else %} grid--3-col-tablet{% endif %}{% if section.settings.swipe_on_mobile %} slider slider--tablet grid--peek{% endif %} collection-list--{{ section.blocks.size }}-items"
<ul class="collection-list contains-card{% if settings.card_style == 'standard' %} contains-card--standard{% endif %} grid grid--1-col{% if section.blocks.size < 5 %} grid--{{ section.blocks.size }}-col-tablet{% else %} grid--3-col-tablet{% endif %}{% if section.settings.swipe_on_mobile %} slider slider--tablet grid--peek{% endif %} collection-list--{{ section.blocks.size }}-items"
id="Slider-{{ section.id }}"
role="list"
>
Expand All @@ -42,7 +42,7 @@
endif
-%}
{%- for block in section.blocks -%}
<li id="Slide-{{ section.id }}-{{ forloop.index }}" class="collection-list__item grid__item{% if section.settings.swipe_on_mobile %} slider__slide{% endif %}" {{ block.shopify_attributes }}>
<li id="Slide-{{ section.id }}-{{ forloop.index }}" class="collection-list__item grid__item{% if section.settings.swipe_on_mobile %} slider__slide{% endif %}{% if block.settings.collection.featured_image == nil %} collection-list__item--no-media{% endif %}" {{ block.shopify_attributes }}>
{% render 'card-collection', card_collection: block.settings.collection , media_aspect_ratio: section.settings.image_ratio, columns: columns %}
</li>
{%- endfor -%}
Expand Down
6 changes: 3 additions & 3 deletions sections/featured-blog.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
assign posts_displayed = section.settings.post_limit
endif
-%}
<div class="blog isolate color-{{ section.settings.color_scheme }} gradient{% if section.settings.heading == blank %} no-heading{% endif %}">
<div class="page-width-desktop{% if posts_displayed < 3 %} page-width-tablet{% endif %} section-{{ section.id }}-padding">
<div class="blog color-{{ section.settings.color_scheme }} gradient{% if section.settings.heading == blank %} no-heading{% endif %}">
<div class="page-width-desktop isolate{% if posts_displayed < 3 %} page-width-tablet{% endif %} section-{{ section.id }}-padding">
<div class="title-wrapper-with-link{% if section.settings.heading == blank %} title-wrapper-with-link--no-heading{% endif %} {% if posts_displayed > 2 %}title-wrapper--self-padded-tablet-down{% else %}title-wrapper--self-padded-mobile{% endif %} title-wrapper--no-top-margin">
<h2 class="blog__title">{{ section.settings.heading | escape }}</h2>

Expand All @@ -46,7 +46,7 @@
{%- if section.settings.blog != blank and section.settings.blog.articles_count > 0 -%}
<slider-component class="slider-mobile-gutter">
<ul id="Slider-{{ section.id }}"
class="blog__posts articles-wrapper grid grid--peek grid--2-col-tablet grid--4-col-desktop slider {% if posts_displayed > 2 %}slider--tablet{% else %}slider--mobile{% endif %}"
class="blog__posts articles-wrapper contains-card{% if settings.card_style == 'standard' %} contains-card--standard{% endif %} grid grid--peek grid--2-col-tablet grid--4-col-desktop slider {% if posts_displayed > 2 %}slider--tablet{% else %}slider--mobile{% endif %}"
role="list"
>
{%- for article in section.settings.blog.articles limit: section.settings.post_limit -%}
Expand Down
2 changes: 1 addition & 1 deletion sections/featured-collection.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
{% endunless %}

<slider-component class="slider-mobile-gutter">
<ul id="Slider-{{ section.id }}" class="grid grid--2-col product-grid{% if products_to_display == 4 or section.settings.collection == blank %} grid--2-col-tablet grid--4-col-desktop{% else %} grid--3-col-tablet{% endif %}{% if products_to_display > 5 %} grid--one-third-max grid--4-col-desktop grid--quarter-max{% endif %}{% if section.settings.collection.all_products_count > 2 and section.settings.swipe_on_mobile and section.settings.products_to_show > 2 %} slider slider--tablet grid--peek{% endif %}" role="list">
<ul id="Slider-{{ section.id }}" class="grid grid--2-col product-grid contains-card{% if settings.card_style == 'standard' %} contains-card--standard{% endif %}{% if products_to_display == 4 or section.settings.collection == blank %} grid--2-col-tablet grid--4-col-desktop{% else %} grid--3-col-tablet{% endif %}{% if products_to_display > 5 %} grid--one-third-max grid--4-col-desktop grid--quarter-max{% endif %}{% if section.settings.collection.all_products_count > 2 and section.settings.swipe_on_mobile and section.settings.products_to_show > 2 %} slider slider--tablet grid--peek{% endif %}" role="list">
{%- for product in section.settings.collection.products limit: section.settings.products_to_show -%}
<li id="Slide-{{ section.id }}-{{ forloop.index }}" class="grid__item{% if section.settings.collection.all_products_count > 2 and section.settings.swipe_on_mobile and section.settings.products_to_show > 2 %} slider__slide{% endif %}">
{% render 'card-product',
Expand Down
2 changes: 1 addition & 1 deletion sections/multicolumn.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
</div>
{% endunless %}
<slider-component class="slider-mobile-gutter">
<ul class="multicolumn-list grid grid--1-col{% if section.blocks.size > 3 and section.settings.image_width != 'full' %} grid--2-col-tablet grid--4-col-desktop{% elsif section.blocks.size > 3 and section.settings.image_width == 'full' %} grid--2-col-tablet{% else %} grid--3-col-tablet{% endif %}{% if section.settings.swipe_on_mobile and section.blocks.size > 1 %} slider slider--mobile grid--peek{% endif %}"
<ul class="multicolumn-list contains-content-container grid grid--1-col{% if section.blocks.size > 3 and section.settings.image_width != 'full' %} grid--2-col-tablet grid--4-col-desktop{% elsif section.blocks.size > 3 and section.settings.image_width == 'full' %} grid--2-col-tablet{% else %} grid--3-col-tablet{% endif %}{% if section.settings.swipe_on_mobile and section.blocks.size > 1 %} slider slider--mobile grid--peek{% endif %}"
id="Slider-{{ section.id }}"
role="list"
>
Expand Down