Implement zero grid spacing and border collapsing #1464
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,16 @@ slider-component.slider-component-full-width { | |
| .slider.slider--mobile.contains-content-container .slider__slide { | ||
| --focus-outline-padding: 0rem; | ||
| } | ||
|
|
||
| .slider.slider--mobile.grid--collapsed .slider__slide { | ||
| --collapsed-offset-y: 0px; | ||
| --collapsed-shadow-offset-y: 0px; | ||
| --collapsed-shadow-offset-x: var(--border-width); | ||
| } | ||
|
|
||
| .slider.slider--mobile.grid--collapsed .slider__slide:first-child { | ||
| --collapsed-shadow-offset-x: 0px; | ||
| } | ||
| } | ||
|
|
||
| @media screen and (min-width: 750px) { | ||
|
|
@@ -103,6 +113,17 @@ slider-component.slider-component-full-width { | |
| .slider.slider--tablet.contains-content-container .slider__slide { | ||
| --focus-outline-padding: 0rem; | ||
| } | ||
|
|
||
| .slider.slider--tablet.grid--collapsed .slider__slide { | ||
| --collapsed-offset-y: 0px; | ||
| --collapsed-shadow-offset-y: 0px; | ||
| --collapsed-shadow-offset-x: var(--border-width); | ||
| } | ||
|
|
||
| .slider.slider--tablet.grid--collapsed .slider__slide:first-child { | ||
| --collapsed-shadow-offset-x: 0px; | ||
| } | ||
|
Comment on lines
+118
to
+125
There was a problem hiding this comment. I think this needs to be applied to the desktop slider as well: There was a problem hiding this comment. Added the slider overrides to desktop grid which should resolve this. There was a problem hiding this comment. I think it's just that the calculation is based on the horizontal space value and when it's set to 0 then it means no extra space for the grid peek. Should be easy to fix in another PR (I can create the issue) by adding There was a problem hiding this comment. Just a question around css variable assignment. I would have thought that applying it to There was a problem hiding this comment. I'm applying it to a Basically, the first rule is overriding the values (from This is the type of selector sliders need to override from grid-collapsed.css.. Also in the corner commit it looks like I had to add even more specificity here and that prompted me to include a comment |
||
|
|
||
| } | ||
|
|
||
| .slider--everywhere { | ||
|
There was a problem hiding this comment. We will also need to change/edit the width of the slides for There was a problem hiding this comment. Could you open a ticket for this with notes on what you think needs to be done for this? I'm not familiar with the new desktop slider. It's odd that the other grid width calculations adjust just fine as is. |
||
|
|
@@ -191,6 +212,20 @@ slider-component.slider-component-full-width { | |
| .slider.slider--desktop.contains-content-container .slider__slide { | ||
| --focus-outline-padding: 0rem; | ||
| } | ||
|
|
||
| .slider.slider--desktop.contains-content-container .slider__slide { | ||
| --focus-outline-padding: 0rem; | ||
| } | ||
| /* specificity here matters */ | ||
| .slider.slider--desktop.grid--collapsed .slider__slide.grid__item { | ||
| --collapsed-offset-y: 0px; | ||
| --collapsed-shadow-offset-y: 0px; | ||
| --collapsed-shadow-offset-x: var(--border-width); | ||
| } | ||
|
|
||
| .slider.slider--desktop.grid--collapsed .slider__slide:first-child { | ||
| --collapsed-shadow-offset-x: 0px; | ||
| } | ||
| } | ||
|
|
||
| @media (prefers-reduced-motion) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| /* | ||
|
There was a problem hiding this comment. This file will eventually be.. a lot. I figure it might be worthwhile to at least define some of the vars and things in comments, but I'm happy to remove them if they're not helpful. |
||
| --collapsed-offset-x/y: the distance to offset an item's border onto the siblings below and to the right | ||
| --collapsed-shadow-offset-x/y: the distance to offset the overlapping shadows caused by offsetting grid items | ||
kmeleta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| */ | ||
| .grid--collapsed-x { | ||
| --collapsed-offset-x: var(--border-width); | ||
| } | ||
| .grid--collapsed-y { | ||
| --collapsed-offset-y: var(--border-width); | ||
| } | ||
|
|
||
| .grid--collapsed-x .grid__item { | ||
| --collapsed-offset-x: calc(var(--border-width) * -1); | ||
| --collapsed-shadow-offset-x: var(--border-width); | ||
| } | ||
| .grid--collapsed-y .grid__item { | ||
| --collapsed-offset-y: calc(var(--border-width) * -1); | ||
| --collapsed-shadow-offset-y: var(--border-width); | ||
| } | ||
|
|
||
| @media screen and (max-width: 989px) { | ||
| /* reset shadow origin along first row/col so its flush with grid edge */ | ||
| .grid--1-col-tablet-down.grid--collapsed-x .grid__item, | ||
| .grid--2-col-tablet-down.grid--collapsed-x .grid__item:nth-child(2n + 1) { | ||
| --collapsed-shadow-offset-x: 0px; | ||
| } | ||
| .grid--1-col-tablet-down.grid--collapsed-y .grid__item:first-child, | ||
| .grid--2-col-tablet-down.grid--collapsed-y .grid__item:nth-child(-n + 2) { | ||
| --collapsed-shadow-offset-y: 0px; | ||
| } | ||
| } | ||
|
|
||
| @media screen and (min-width: 990px) { | ||
| /* reset shadow origin along first row/col so its flush with grid edge */ | ||
| .grid--1-col-desktop.grid--collapsed-x .grid__item, | ||
| .grid--2-col-desktop.grid--collapsed-x .grid__item:nth-child(2n + 1), | ||
| .grid--3-col-desktop.grid--collapsed-x .grid__item:nth-child(3n + 1), | ||
| .grid--4-col-desktop.grid--collapsed-x .grid__item:nth-child(4n + 1), | ||
| .grid--5-col-desktop.grid--collapsed-x .grid__item:nth-child(5n + 1), | ||
| .grid--6-col-desktop.grid--collapsed-x .grid__item:nth-child(6n + 1) { | ||
| --collapsed-shadow-offset-x: 0px; | ||
| } | ||
| .grid--1-col-desktop.grid--collapsed-y .grid__item:nth-child(1), | ||
| .grid--2-col-desktop.grid--collapsed-y .grid__item:nth-child(-n + 2), | ||
| .grid--3-col-desktop.grid--collapsed-y .grid__item:nth-child(-n + 3), | ||
| .grid--4-col-desktop.grid--collapsed-y .grid__item:nth-child(-n + 4), | ||
| .grid--5-col-desktop.grid--collapsed-y .grid__item:nth-child(-n + 5), | ||
| .grid--6-col-desktop.grid--collapsed-y .grid__item:nth-child(-n + 6) { | ||
|
There was a problem hiding this comment. I know that the Featured Collection section and Collection template have a 5 column product grid setting. Is there a template or section that has a 6 column option? There was a problem hiding this comment. I'd love to cut out one those cases out of the collapsed code, but I believe multicolumn allows 6. |
||
| --collapsed-shadow-offset-y: 0px; | ||
| } | ||
| } | ||
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.
Do you need to include the
contains-xyzhere for specificity?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.
We discussed this a bit already but will respond with my thoughts here for ref. Including the contains classes here was a little bit of premature optimization to protect against future possible cases like
content-containergrid items that also have children using.global-media-settings. We wouldn't want any collapsed grid styles to apply to the deeper children. Maybe this won't ever occur, but I'm not certain.This does unnecessarily create slightly higher specifically right now, but I see the collapsed grid as something that's hard to test at scale and likely to have regressions go unnoticed. For these reasons I like the idea of a little additional futureproof-ness. I don't necessarily think the collapsed grid styles being more difficult to override is a totally bad thing in some cases.
I think for now we can include the removal of this a follow up enhancement to consider. I'm sure there are other things that will also fit into this category.