Update slider shadow padding implementation #1292

kmeleta · 2022-01-31T18:06:01Z

Why are these changes introduced?

We previously added additional padding above and below sliders to compensate for shadow offset. This change makes it possible to remove the additional bottom padding specifically for sliders that contain standard style cards.

What approach did you take?

Previously, slider shadow padding was applied to the <ul class="slider"> element. I've changed the approach to put this padding on the <li class="slider__slide"> elements instead. This was mostly to accommodate a case in collection list where empty collection cards still need the bottom padding regardless of which card style is used (see here)

In order to better scope generic shadow variables and account for exception cases I've created new classes to describe the type of children a slider/grid contains. I think these could be utilized more to further simplify global setting variable inheritance, but for now I'm using only where relevant to this issue.

Using the same method, I also added an exception that prevents the previously added .5rem for focus outlines on content-container sliders, which don't receive focus.

Other considerations

Probably a UX decision, but instead of just removing bottom padding we could add it between the card image and the title. I don't think the further separation of these two elements looks particularly good, but maybe for preserving contrast we should consider it. Perhaps this is better solved by providing a title spacing setting or something.

Struggled naming the new contains classes. Other ideas welcome.

This PR doesn't modify the actual shadow offset/blur calculations we're using for the padding. We can probably still do more to improve this but should be considered separately from this PR.

Testing notes:

Check for padding/margin related regressions in all slider sections
Check with positive and negative vertical shadow offsets

Demo links

Editor

Checklist

ludoboludo

Looking good, left a few comments.

ludoboludo · 2022-01-31T22:03:26Z

assets/component-slider.css

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


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

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.

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

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.

ludoboludo · 2022-01-31T22:12:14Z

assets/base.css

+.contains-card--standard,
+.contains-card--card,


Since you only have modifiers atm, I would suggest applying those variables to .contains-card and add that class where you've added .contains-card--style.

I went back and forth so much on these. I ultimately didn't add it because it wouldn't be useful, as used currently. But it theoretically makes sense. I may opt to just wait til we're making more use of these contexts (grid gap changes and make sure I'm still going to need them the way I think.

I think you could have .contains-card and .contains-card--standard 🤔 I think it'd be nice to follow BEM

I'm not sure it's worth the unused additional class in the dom, but I've added it to be more correct 2af6578

Oh what I meant is maybe contains-card could be the default using the current styles from contains-card--card. And then only have one "modifier" with --standard. What do you think 🤔

Ahh, I think I like that actually. Does this make sense? d7f333e

Yeah I think so 👍

assets/base.css

ludoboludo · 2022-01-31T22:32:01Z

assets/component-slider.css

  }

  .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));


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

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

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.

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

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.

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.

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

assets/component-slider.css

melissaperreault

Thank you Ken!
Replied where needed and approving this PR as it is currently fixing the issue described. I would solve any follow up items in another PR/issue. Happy to pair and align on what needs alignment!

ludoboludo

Looking good 👍

tyleralsbury

This all looks good to me. I'm wondering if there would be any value in turning the calculation into a reusable class with a name that tells us what it's there for - it's a bit complex to get your head around unless you've got the context.

No need at this precise moment, but something to consider as part of the refactoring we're looking to do.

kmeleta · 2022-02-03T19:49:05Z

This all looks good to me. I'm wondering if there would be any value in turning the calculation into a reusable class with a name that tells us what it's there for - it's a bit complex to get your head around unless you've got the context.

Thanks @tyleralsbury I did try to break it up a bit into vars with somewhat descriptive labels like --shadow-padding-top. Not actually sure how better to describe the purpose without leaving a comment. I didn't make it a class yet just because its only use is in sliders. However, I think we may want to further tweak the calculation anyway so we can consider it more closely then.

tyleralsbury · 2022-02-03T19:58:00Z

Thanks @tyleralsbury I did try to break it up a bit into vars with somewhat descriptive labels like --shadow-padding-top. Not actually sure how better to describe the purpose without leaving a comment. I didn't make it a class yet just because its only use is in sliders. However, I think we may want to further tweak the calculation anyway so we can consider it more closely then.

It's probably okay as-is if it's only used for sliders. My concern would be this calculation popping up in different places throughout the theme, making it harder to maintain. If this isn't something we need to worry about, then I think the named variable is good enough.

* Update slider shadow padding implementation * Adjust blog isolate * bem * bem adjust

Update slider shadow padding implementation

35fbb7a

kmeleta marked this pull request as ready for review January 31, 2022 18:09

kmeleta mentioned this pull request Jan 31, 2022

Prevent excess padding on standard card sliders #1248

Closed

5 tasks

ludoboludo self-requested a review January 31, 2022 20:57

ludoboludo reviewed Jan 31, 2022

View reviewed changes

kmeleta requested a review from melissaperreault January 31, 2022 23:15

melissaperreault previously approved these changes Feb 1, 2022

View reviewed changes

Adjust blog isolate

9fbc384

kmeleta dismissed melissaperreault’s stale review via 9fbc384 February 1, 2022 15:22

kmeleta added 2 commits February 1, 2022 08:28

bem

2af6578

bem adjust

d7f333e

ludoboludo approved these changes Feb 1, 2022

View reviewed changes

tyleralsbury self-requested a review February 3, 2022 15:21

tyleralsbury approved these changes Feb 3, 2022

View reviewed changes

kmeleta merged commit 8fa3634 into main Feb 3, 2022

kmeleta deleted the fix-slider-card-padding branch February 3, 2022 19:49

LucasLacerdaUX mentioned this pull request Apr 8, 2022

Product thumbnails are stretched on mobile (Product page) #1565

Open

phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024

Update slider shadow padding implementation (Shopify#1292)

c1439d5

* Update slider shadow padding implementation * Adjust blog isolate * bem * bem adjust

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update slider shadow padding implementation #1292

Update slider shadow padding implementation #1292

kmeleta commented Jan 31, 2022 •

edited

Loading

ludoboludo left a comment

ludoboludo Jan 31, 2022

kmeleta Jan 31, 2022

kmeleta Jan 31, 2022

melissaperreault Feb 1, 2022 •

edited

Loading

ludoboludo Jan 31, 2022

kmeleta Feb 1, 2022

ludoboludo Feb 1, 2022

kmeleta Feb 1, 2022

ludoboludo Feb 1, 2022

kmeleta Feb 1, 2022

ludoboludo Feb 1, 2022

ludoboludo Jan 31, 2022

ludoboludo Jan 31, 2022

kmeleta Jan 31, 2022 •

edited

Loading

melissaperreault Feb 1, 2022

kmeleta Feb 1, 2022 •

edited

Loading

kmeleta Feb 1, 2022 •

edited

Loading

kmeleta Feb 7, 2022

melissaperreault left a comment

ludoboludo left a comment

tyleralsbury left a comment

kmeleta commented Feb 3, 2022

tyleralsbury commented Feb 3, 2022

		.contains-card--standard,
		.contains-card--card,

Update slider shadow padding implementation #1292

Update slider shadow padding implementation #1292

Conversation

kmeleta commented Jan 31, 2022 • edited Loading

ludoboludo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melissaperreault Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmeleta Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmeleta Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

kmeleta Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melissaperreault left a comment

Choose a reason for hiding this comment

ludoboludo left a comment

Choose a reason for hiding this comment

tyleralsbury left a comment

Choose a reason for hiding this comment

kmeleta commented Feb 3, 2022

tyleralsbury commented Feb 3, 2022

kmeleta commented Jan 31, 2022 •

edited

Loading

melissaperreault Feb 1, 2022 •

edited

Loading

kmeleta Jan 31, 2022 •

edited

Loading

kmeleta Feb 1, 2022 •

edited

Loading

kmeleta Feb 1, 2022 •

edited

Loading