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.

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

Implement zero grid spacing and border collapsing #1464

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

kmeleta
Copy link
Contributor

@kmeleta kmeleta commented Mar 7, 2022

PR Summary:

Adds the ability to use 0px grid spacing in the global settings.

Why are these changes introduced?

Fixes #922

These changes lower the minimum value of the grid spacing setting from 4px to 0px and allows for borders on grid items to "collapse" (not double up against adjacent items). The double border and corner radius effect is why we didn't initially enable 0px spacing.

Note: Right now, this PR only addresses border collapsing. The corner radius collapsing aspect also needs to be accounted for, but is being handled separately in this PR #1478

What approach did you take?

Achieving the collapsed borders
tl;dr I've chosen to "collapse" the borders by applying negative margins equal to the border width to grid items. These margins are applied to applicable grid children via --collapsed-offset-x/y variables.

Achieving the collapsed borders continued... A small caveat with this, right now it's actually the `card` or `content-container` element within a grid__item that I'm offsetting, rather than the grid item itself. This is because our grid doesn't currently store the number of columns in a css var and we'd need that info to calculate the proper counter-offset for the grid list. I see this as a change we can make after further grid refactoring if we want to, but not having to apply margin on the list items probably reduces the chances of running into conflicting existing margin styles.

In addition to `--collapsed-offset-x` there's also a `collapsed-shadow-offset-x` variable being used. This is used to modify the bounds of the shadow [pseudo element] so they don't overlap with adjacent shadows.

I've looked at many other possible approaches, such as removing certain borders, halving borders along vertices, using a grid gap and a underlaid background color to create a border, hacking box-shadow and/or drop-shadow filters, etc but I found these all to be either dead ends or more complicated and fragile. If there's an approach I dismissed that might be worth another look, please let me know.

Added new css file grid-collapsed.css
Most of the selectors related to collapsed grids have been added to a new css file that only gets loaded if the global spacing setting is set to 0. The css needed right now may not warrant its own css file, but the amount of code needed will dramatically increase when the corner collapsing is factored in.

Added new grid--collapsed-x/y classes
Vertical and horizontal spacing settings can be independently set to 0 so we need to be able to handle all 3 scenarios (x-only, y-only, both). The appropriate x or y classes will be applied to inform the necessary selectors. One caveat is that when card style standard is selected, we never render the grid--collapsed-y on grids that contain cards because it's not applicable. Edit: We always render all applicable classes and account for the standard card exceptions with additional selectors

Collage, Main blog, Product media are special cases
Product media uses the grid class but no grid--#-col classes, while collage and main blog don't use grid at all. These all have "irregular" 2 column layouts with their own concerns and variations (collage in particular). I'm including the border collapse styles for each of these in their respective css files, since they don't directly relate to the general grid classes referenced in grid-collapsed.css.

Other considerations

Known issue: Standard cards
This is terrible looking. It will look even weirder with corner collapsing. Bottomline is that standard style cards are not always going to work well in a collapsed grid. Melissa and I have spoken about this and the related product title issue which needs to be considered. Potentially with a setting that allows for different spacing around product titles.

Known issue: Product media heights
This isn't caused by anything in this collapsed grid changes, but it doesn't look any better in a collapsed grid setting. A possible solution to be considered could be allowing for aspect ratio control like in product card grids.

Known issue: Shadow gaps
Due to the nature of box-shadows in css, a gap may be visible at higher opacities when blur is applied. I will investigate to see if we can try to compensate for amount of blue, but can't be certain this is possible.

Known issue: Article/collection card shadows
When a collection grid or article grid have a mix of standard cards and text-only cards, we can't predict the needed offsets at each intersection. With shadows applied, overlaps will be noticeable where text-only cards meet another card. If all standard cards have images, this will not be a problem. Also does not affect product cards. It's possible this problem could be solved in a some cases, but an investigation will be needed and can be done later.

Known exception: Product thumbnails
These don't use the global spacing settings and don't believe they should. As such, they will not receive the collapsed grid styles.

Testing steps/scenarios

Affected areas..

  • Main collection
  • Search results
  • Main collections list
  • Collection list section
  • Featured collection
  • Multicolumn
  • Featured blogs
  • Main blog
  • Collage
  • Product media gallery

Permutations..

  • zero horizontal spacing
  • zero vertical spacing
  • zero horizontal and vertical spacing
  • with and without sliders enabled
  • card/standard style cards
  • various border widths
  • vrious number of columns
  • with drop shadows

Demo links

Checklist

@kmeleta kmeleta force-pushed the grid-collapse branch 3 times, most recently from 5d36f32 to 7aaa2dd Compare March 8, 2022 18:29
Comment on lines 50 to 58
.grid--collapsed-y.contains-card .card-wrapper {
height: calc(100% - var(--collapsed-offset-y));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an extension of the height: 100% we already apply to cards that is needed for the bottom border to align properly in a collapsed grid. It's not ideal but I don't see it as too much of an issue. It could be avoided if we applied the offset to the grid items rather than the cards, media, etc within. See note in the PR description for more.

@@ -0,0 +1,51 @@
/*
Copy link
Contributor Author

@kmeleta kmeleta Mar 8, 2022

Choose a reason for hiding this comment

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

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.

@kmeleta kmeleta marked this pull request as ready for review March 8, 2022 19:17
@sofiamatulis sofiamatulis self-assigned this Mar 9, 2022
Copy link
Contributor

@sofiamatulis sofiamatulis left a comment

Choose a reason for hiding this comment

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

Nice! Did a first pass and just noted some things

@kmeleta kmeleta force-pushed the grid-collapse branch 3 times, most recently from a2b7c20 to f7e0b3b Compare March 9, 2022 23:24
@kmeleta kmeleta mentioned this pull request Mar 10, 2022
24 tasks
YoannJailin
YoannJailin previously approved these changes Mar 10, 2022
Copy link

@YoannJailin YoannJailin left a comment

Choose a reason for hiding this comment

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

Looks amazing to me!

Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

First round of review, I will need to test further in different browsers and what not.

@@ -167,7 +167,7 @@
{
"type": "range",
"id": "spacing_grid_horizontal",
"min": 4,
"min": 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Something I'm noticing for sliders is that we night need to adjust the width calculation of a slide to always show a peek effect: video

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks we're adding a hardcoded value on mobile to always keep the grid peek: screenshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like it's just a featured collection desktop slider issue. Tablet and mobile sliders seem like adapt fine with their existing styles. I'm not modifying any and grid item widths in these PRs, so unless the issue is caused by the collapsed classes or offset vars I'm using, I'd rather not deal with that here.

@kmeleta kmeleta force-pushed the grid-collapse branch 5 times, most recently from d01cc2b to 22de63c Compare March 16, 2022 17:35
YoannJailin
YoannJailin previously approved these changes Mar 17, 2022
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

I'm not too sure how else we could approach this situation for border collapsing.

It does add a lot of logic/code for a scenario that might not be that common to come across 🤔

Maybe we could group more of the styling in that grid-collapsed.css file. Otherwise that's a lot of unused CSS.

Noticed also this:

  • when we have a 3d model on the product page it seem to break the zero grid effect due to the view in your space button. That's on my tablet.
  • on my android phone, it's very settle but figured i'd mention it. There is a very subtle overlap of the border from the item on the right: screenshot

Comment on lines +118 to +125
--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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be applied to the desktop slider as well:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the slider overrides to desktop grid which should resolve this.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 - 2rem to the calculation of the grid item width.

.slider.slider--tablet.grid--collapsed .slider__slide:first-child {
--collapsed-shadow-offset-x: 0px;
}

}

.slider--everywhere {
Copy link
Contributor

Choose a reason for hiding this comment

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

We will also need to change/edit the width of the slides for .slider--desktop.grid--4-col-desktop .grid__item. As the slider on desktop isn't showing the peek anymore. But could be a quick follow up issue. same screenshot as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +19 to +27
{%- liquid
if settings.spacing_grid_horizontal == 0 and settings.spacing_grid_vertical == 0
assign collapsed_class = 'blog--collapsed blog--collapsed-x blog--collapsed-y'
elsif settings.spacing_grid_horizontal == 0
assign collapsed_class = 'blog--collapsed blog--collapsed-x'
elsif settings.spacing_grid_vertical == 0
assign collapsed_class = 'blog--collapsed blog--collapsed-y'
endif
-%}
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 could just use the snippet everywhere if we go that route. So doing something like:
{{ render 'collapsed-classes', target: 'blog/grid/collage' }}

And then within the snippet use what's passed as a target + --collapsed 🤔

{{ target | append: '--collapsed' }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's exactly what I described to Sofia as something I wasn't really excited to do. I suppose it's not too bad without needing to also factor in the card type exception, but I think I'm fine keeping them copied in collage and blog. I'd like to see blog sharing grid styles in the future so I don't want to go out of our way too much for it. If we change our mind it can always be done in a cleanup PR.

@sofiamatulis
Copy link
Contributor

Can you fix the conflicts before I re-review? 👀

@kmeleta
Copy link
Contributor Author

kmeleta commented Mar 31, 2022

@sofiamatulis @ludoboludo I've rebased onto the quick add and vertical filter commits. It was a little ugly so probably pay a little more attention to main product, main product grid, and search next time you look. The corner commit has also been rebased with those changes.

Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Noticed a couple things.

There isn't a margin collection between sections:

This might not be coming from this PR. On collapsible content I'm getting no top border for some reasons:

@ludoboludo
Copy link
Contributor

Ah just noticed this: #1530 👍 which address one the concern I shared above.

@kmeleta
Copy link
Contributor Author

kmeleta commented Apr 1, 2022

Ah just noticed this: #1530 👍 which address one the concern I shared above.

Yes, I maybe could have made it more clear but since the grid spacing settings don't actually directly affect section-section spacing, it doesn't really belong here. That other PR is technically open but still needs an initial UX walkthrough.

This might not be coming from this PR. On collapsible content I'm getting no top border for some reasons

I'm fairly certain there's nothing in here that should be causing that (I can't even seem to reproduce the issue). I don't remove any borders at all to achieve the collapsed effect and I definitely didn't touch any accordion styles here. If the improper border style in question was coming from the global content-container styles or something, I'd be suspicious.

Side note: I believe allowing accordions to use the grid spacing settings has been captured in an issue, but right now that component should have no relation to this PR. @ludoboludo

ludoboludo
ludoboludo previously approved these changes Apr 11, 2022
@martinamarien martinamarien self-requested a review April 11, 2022 20:29
Copy link
Contributor

@martinamarien martinamarien left a comment

Choose a reason for hiding this comment

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

Looking good Ken.
I've done an initial pass, but I haven't had the chance to test sliders yet.
A few things we need to implement before merging this - like adding the collapsed grid styles to password.liquid.

Others, like specificity and the collage shadow issue, I'll let you decide. I'd be fine with looking at those in separate PRs. Especially the collage one, given that it seems to be exclusively related to collage section, and not the other sections/grids/sliders.

.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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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'd love to cut out one those cases out of the collapsed code, but I believe multicolumn allows 6.


.slider.slider--tablet.grid--collapsed .slider__slide:first-child {
--collapsed-shadow-offset-x: 0px;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question around css variable assignment.
Do you need to apply it to the slider_slide?

I would have thought that applying it to slider.slider--tablet.grid--collapsed would mean that the slide would inherit the correct value. And then you could use slider_slide:first-child to overwrite it in certain situations.

  .slider--tablet.grid--collapsed {
    --collapsed-offset-y: 0px;
    --collapsed-shadow-offset-y: 0px;
    --collapsed-shadow-offset-x: var(--border-width);
  }
  
  .slider--tablet.grid--collapsed .slider__slide:first-child {
    --collapsed-shadow-offset-x: 0px;
  }

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'm applying it to a slider__slide just because the selectors these are attempting to override is applied to slider__slide (grid__item).

Basically, the first rule is overriding the values (from grid-collapsed.css) on all the slides purposefully. For example.. If a slider grid has .grid--2-col-tablet-down then every other item would have 0px shadow offset. Since an active slider wouldn't be in a 2 column format, we want all items to have the same x offset, except for the first.

This is the type of selector sliders need to override from grid-collapsed.css..

/* 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;
  }

Also in the corner commit it looks like I had to add even more specificity here and that prompted me to include a comment /* certain specificity needed for all rules below, modify with care */.

Comment on lines +2839 to +2857
.grid--collapsed-x.contains-content-container .content-container,
.grid--collapsed-x.contains-media .global-media-settings {
margin-right: var(--collapsed-offset-x);
}

.grid--collapsed-y.contains-content-container .content-container,
.grid--collapsed-y.contains-media .global-media-settings {
margin-bottom: var(--collapsed-offset-y);
}

.grid--collapsed-x.contains-content-container .content-container:after,
.grid--collapsed-x.contains-media .global-media-settings:after {
margin-left: var(--collapsed-shadow-offset-x);
}

.grid--collapsed-y.contains-content-container .content-container:after,
.grid--collapsed-y.contains-media .global-media-settings:after {
margin-top: var(--collapsed-shadow-offset-y);
}
Copy link
Contributor

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-xyz here for specificity?

.grid--collapsed-y .content-container:after,
.grid--collapsed-y .global-media-settings:after {
  margin-top: var(--collapsed-shadow-offset-y);
}

Copy link
Contributor Author

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-container grid 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.

grid-column: 1 / span 2;
grid-row: span 2;
}

.collage__item--left:nth-child(3n - 2):last-child {
.collage__item--left:first-child:last-child {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for updating this. I find this very readable now. 🙏🏻

Comment on lines +191 to +195
.collage--collapsed-x .collage-card {
width: calc(100% + var(--card-border-width));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me why we won't need to apply this style to .card-wrapper, like we do for height. Can you explain it? Thanks 🙏🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I think this is here to override the .collage-item > * style { width: 100% } at the top of collage.css. It's possible I could be reseting to width: auto instead, but this probably seemed a little more explicit at the time. The difference I'm sure has to do with where the actual border styles are being applied relative to the where any width/height dimensions are being applied. The height: 100% calc stuff is needed for both because we want cards to stretch to fill the row height (while still using border-box sizing).

Comment on lines 195 to 211
.collage--collapsed-x {
margin-right: var(--card-border-width);
}
.collage--collapsed-y {
padding-bottom: var(--card-border-width);
}
.collage--collapsed-x :where(.card, .collage-card) {
margin-right: calc(var(--card-border-width) * -1);
}
.collage--collapsed-y :where(.card, .collage-card) {
margin-bottom: calc(var(--card-border-width) * -1);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.collage--collapsed-x {
margin-right: var(--card-border-width);
}
.collage--collapsed-y {
padding-bottom: var(--card-border-width);
}
.collage--collapsed-x :where(.card, .collage-card) {
margin-right: calc(var(--card-border-width) * -1);
}
.collage--collapsed-y :where(.card, .collage-card) {
margin-bottom: calc(var(--card-border-width) * -1);
}
.collage--collapsed-x {
margin-right: var(--card-border-width);
}
.collage--collapsed-y {
padding-bottom: var(--card-border-width);
}
.collage--collapsed-x :where(.card, .collage-card) {
margin-right: calc(var(--card-border-width) * -1);
}
.collage--collapsed-y :where(.card, .collage-card) {
margin-bottom: calc(var(--card-border-width) * -1);
}

TIL :where. I've never used this.
Theoretically, could we use this above too?
I'm not asking you to change anything, just more out of curiosity.

.collage--collapsed-y :where(.card-wrapper, .collage-card) {
  height: calc(100% + var(--card-border-width));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep it certainly could. Likely I wasn't going to use :where at all originally, then decided with the number of lines being added, I should use it, and ended up missing some instances. I can update with my next edits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I made other changes to this stuff anyway, but when I originally went to incorporate :where I realized that :where is the one that doesn't add specificity and overriding the height needed more. :is() is different I think but I thought it was weird and compounds specificity or something with each comma separated selector.

.collage--collapsed-x .collage__item--left:first-child .card--standard .card__inner:after {
margin-right: var(--card-border-width);
}
.collage--collapsed-x .collage__item--right:last-child :where(.card--card, .collage-card):after,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.collage--collapsed-x .collage__item--right:last-child :where(.card--card, .collage-card):after,
.collage--collapsed-x .collage__item--right:last-child :where(.card--card, .collage-card):after,

Comment on lines 212 to 249
.collage--collapsed-x .collage__item--right:last-child :where(.card--card, .collage-card):after,
.collage--collapsed-x .collage__item--right:last-child .card--standard .card__inner:after {
margin-left: var(--card-border-width);
}

.collage--collapsed-y .collage__item--left:nth-child(3) :where(.card--card, .collage-card):after,
.collage--collapsed-y .collage__item--left:nth-child(3) .card--standard .card__inner:after {
margin-top: var(--card-border-width);
}

@media screen and (min-width: 750px) {
.collage--collapsed-y .collage__item--right:first-child :where(.card--card, .collage-card):after,
.collage--collapsed-y .collage__item--right:first-child .card--standard .card__inner:after {
margin-bottom: var(--card-border-width);
}
}

@media screen and (max-width: 749px) {
.collage--collapsed-y:not(.collage--mobile) .collage__item :where(.card--card, .collage-card):after,
.collage--collapsed-y:not(.collage--mobile) .collage__item .card--standard .card__inner:after,
.collage--collapsed-y.collage--mobile .collage__item--right:nth-child(3) :where(.card--card, .collage-card):after,
.collage--collapsed-y.collage--mobile .collage__item--right:nth-child(3) .card--standard .card__inner:after {
margin-top: var(--card-border-width);
}

.collage--mobile.collage--collapsed-y .collage__item--left:first-child :where(.card--card, .collage-card):after,
.collage--mobile.collage--collapsed-y .collage__item--left:first-child .card--standard .card__inner:after {
margin-bottom: var(--card-border-width);
}

.collage--mobile.collage--collapsed-x .collage__item--left:nth-child(3) :where(.card--card, .collage-card):after,
.collage--mobile.collage--collapsed-x .collage__item--left:nth-child(3) .card--standard .card__inner:after {
margin-left: var(--card-border-width);
}

.collage--mobile.collage--collapsed-x .collage__item--right:first-child :where(.card--card, .collage-card):after,
.collage--mobile.collage--collapsed-x .collage__item--right:first-child .card--standard .card__inner:after {
margin-right: var(--card-border-width);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For this content, I am going to recommend that we include a comment.
Why?: It wasn't obvious to me why we needed this. When I removed the margin property, it didn't have any effect, until I added shadows. Then I realized it was being used to prevent the shadow overlap. I think this should be added as a comment, just so that it's extremely clear to anyone looking at this why it's needed.

Copy link
Contributor

@martinamarien martinamarien Apr 18, 2022

Choose a reason for hiding this comment

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

I did find an issue with shadows for collage.
It looks like the margin right and margin bottom are causing the shadow to behave incorrectly. As the border width gets bigger, it starts to overlap and then overtake the shadow. But does not seem to be an issue for negative shadows.

Screen Shot 2022-04-18 at 12 16 02 PM

On mobile/tablet, I can reproduce with this X and Y collapsed borders and Card and Standard card styles.

Screen Shot 2022-04-18 at 12 02 46 PM

Screen Shot 2022-04-18 at 12 01 05 PM

I can also replicate it on Desktop for collapse--x, but only for the :first-child. When the desktop layout is Left or Right large. This is happening regardless of block # (1, 2 or 3), but differs on which border is miscalculated.

Left large
Horizontal = 0
Screen Shot 2022-04-18 at 12 07 59 PM

Horizontal != 0
Screen Shot 2022-04-18 at 12 08 23 PM

Right large:
Screen Shot 2022-04-18 at 12 10 24 PM

Copy link
Contributor

@martinamarien martinamarien Apr 18, 2022

Choose a reason for hiding this comment

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

Follow up:
When there is a large border, the shadow can disappear on mobile/desktop on the :first-child / first block (standard card + card style). And the text of standard style cards can become overlapped with the border.

Screen Shot 2022-04-18 at 12 32 48 PM

Screen Shot 2022-04-18 at 12 29 25 PM

Screen Shot 2022-04-18 at 12 30 52 PM

Copy link
Contributor Author

@kmeleta kmeleta Apr 21, 2022

Choose a reason for hiding this comment

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

Good finds here. There were a couple things going on here that I wasn't fully aware of. The thing where a large border was taking away vertical space for example, was actually an issue outside of collage too (in collection and article cards) and definitely needed to be fixed.

Also fixed some cases where a shadow offset was being made for items where it shouldn't be, which caused the shadow to "disappear". I think some of these were accidentally fixed in the corner commit so I ported over some of those to this commit.

I definitely want to make sure those types of shadow issues are fixed for card-style so hopefully those look better now. Though I did fix a couple related standard style shadow issues as well. I know more exist with certain combinations (like this) but would require bigger updates which can be considered in follow ups.

@kmeleta kmeleta force-pushed the grid-collapse branch 2 times, most recently from e49cd61 to f89415b Compare April 21, 2022 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Global settings] Adjust adjacent borders and corners
7 participants