-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[3D lift animation] Raise hovered card above adjacent cards #2589
Conversation
9c9a2fd
to
2b1678b
Compare
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.
I noticed while playing with shadows that when you have some horizontal shadow using a negative value, it's now showing that shadow on top of the previous card when it wasn't before.
Though not introduced in that PR but with 3D lift. Was that noted before and we're ok with it? I don't think it's a blocker but something for the merchants to figure out.
Due to this: screenshot
assets/base.css
Outdated
@@ -3353,6 +3353,11 @@ details-disclosure > details { | |||
transform: rotate(0.5deg); /* Less intense rotation for collage items. */ | |||
} | |||
|
|||
.animate--hover-3d-lift .grid__item:hover, |
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.
applying it to grid__item
scares me a little. It feels to generic/general. Lots of elements that aren't getting that effect I believe would still be using this class.
So maybe we could be more specific at targeting containers of elements that do have the 3d lift applied to them 🤔
A couple ways we could go about it:
- use a selector that is yet to be supported on all browsers, so the experience isn't the same everywhere but it's a "cute" fix not specifically functional. So using grid__item:hover:has(.card, .collection__card, etc)`
- target something a bit more specific.
collection-list__item
,.product-grid .grid__item
,.complementary-slide li
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.
👍 I targeted more specific classes in 777fa46. I think I got them all: product grids, collection lists, collage items, blog articles, and complementary products. Let me know if you can think of any other contexts we should hit.
.animate--hover-3d-lift .grid__item:hover, | ||
.animate--hover-3d-lift .collage__item:hover { | ||
z-index: 1; /* Make sure the hovered card is the topmost card. */ | ||
} |
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.
For general shadows we're applying some padding to account for the shadow spread. Which we're not with this and it can cut off the shadow, specifically on sliders: video
Something similar could be useful to look into. Maybe as a follow up for the next release.
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.
.animate--hover-3d-lift .grid__item:hover, | ||
.animate--hover-3d-lift .collage__item:hover { | ||
z-index: 1; /* Make sure the hovered card is the topmost card. */ | ||
} |
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.
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.
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.
z-index: 2;
seem to create its own issue with the search template:
I don't think it's a huge deal. But ideally it doesn't happen. I think one way to go about it, would be to apply on the product-grid
, isolation: isolate;
so we basically scope the stacking elements and their z-index to this wrapper.
I'n not seeing a negative impact in adding the property so far.
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.
Ah, good call. Added in 0a95ba2 — I haven't found any negative repercussions either. I could scope it to just template-search
if we're nervous though.
Thanks @kjellr @ludoboludo for opening this ticket! Currently, it looks like we use box shadow on each of the cards - this creates issues whenever a shadow is larger than the gap between elements/cards because we can't separate the 'z-index' of the card and its shadow. We already use the drop shadow for the card hover animation. I wonder - is it an option to have a drop shadow on the parent element (i.e. grid)? (Drop shadow vs. box shadow) I got this working in a Dawn test theme by adding a simple drop shadow filter to the collection grid like this:
By the way - I noticed that once we sorted out the issue with adjacent cards, having the card hover shadow below adjacent cards actually looks pretty good. This may actually be a viable option because it may prevent cards becoming unreadable when overlapped by shadows?
PS:
|
@@ -3200,7 +3200,6 @@ details-disclosure > details { | |||
.animate--hover-3d-lift .deferred-media .deferred-media__poster-button { | |||
transition: transform var(--duration-long) ease, box-shadow var(--duration-long) ease; | |||
transform-origin: center; | |||
z-index: 1; |
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.
@ludoboludo Just a heads up: I think we're ok removing this rule entirely for now. We're overriding it for cards with the new changes below, and I don't think it was actually necessary for buttons anyway — they appear to work ok in my testing as-is.
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.
This will fix the 3D Lift portion of #2596.
Thanks @bredowmax!
We’re tracking some regressions related to card shadows over in #2596. Applying a shadow to the parent rather than the cards themselves (whether with
That's similar to how the 3D effect works today, without this PR, right? I think I tend to prefer the hover shadow appearing over the cards, so that it reinforces the "lift" behavior. |
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.
LGTM! 🚢 🎉
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.
I think it's looking good 👍
.animate--hover-3d-lift .collage__item:hover, | ||
.animate--hover-3d-lift .blog-articles .article:hover, | ||
.animate--hover-3d-lift .complementary-slide li:hover { | ||
z-index: 2; /* Make sure the hovered card is the topmost card. */ |
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.
One minor downside to this is that z-index
isn't (and can't really be) animated along with the rest of the shadow behavior. So when there's a dark shadow that's also big enough to overlap adjacent cards, the shadow kinda "pops" up above on hover, making the animation feel less smooth. For example:
pop.mp4
This isn't too bad, and it also seems like an edge case to me. What we have here should work nicely for the most common types of shadow usage, and it won't effect any of our themes by default.
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.
Yeah, and I think we saw that safari is a bit wonky at showing the shadow on top and removing it. But as you said I think it's an edge case that can be fixed by removing some shadows from cards in the general settings or having more space between them.
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.
Just one comment to consider. Personally I'm not sure the flickering effect is better than the shadows being below, but I will leave it to you if you would prefer it. We could add a follow-up issue to revisit if we think it's worthwhile.
.animate--hover-3d-lift .collage__item:hover, | ||
.animate--hover-3d-lift .blog-articles .article:hover, | ||
.animate--hover-3d-lift .complementary-slide li:hover { | ||
z-index: 2; /* Make sure the hovered card is the topmost card. */ |
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.
Giving the hovered item a bigger z-index does tend to create a flickering effect in some cases since z-index is either 0 or 2. It would be pretty tricky to animate the shadow, though. I can imagine fancy and complicated ways of doing it using two shadows: a top shadow and a bottom shadow that fade in and out depending on hover.
Screen.Recording.2023-05-02.at.3.00.15.PM.mov
Yeah, I know what you mean. I went back and forth on it myself. We definitely need to do the minor As noted up here, the super heavy shadows that cause this issue are kinda edge cases. They're nothing like what we do in any of our themes by default. When the shadows are smaller, lighter, or nonexistent, this change introduces a positive effect of actually lifting the card above other neighboring cards. I think that's worthwhile for now, and we can continue cleaning up iteratively after release. |
…2589) * Raise hovered card above adjacent cards. * Target more specific classes. * Increase the z-index so that complementary slides work. * Only change z-index for hovered articles. * Remove unnecessary z-index rule. * Ensure cards don't overlap search popover. --------- Co-authored-by: Ludo <ludo.segura@shopify.com>
…hopify#2589)" This reverts commit 641ba8e.
* shopify/main: (59 commits) [Announcement bar] Add social icons (Shopify#2497) Update theme version to match the pubic release (Shopify#2698) Add release/v10.0.0 branch fixes to main (Shopify#2693) fix default UI for dropdown and mega menu (Shopify#2644) Fix link formatting in Related Products heading (Shopify#2680) Update 2 translation files (Shopify#2637) Enable gift card recipient form by default on featured product section (Shopify#2666) Gift cards/enable recipient form by default (Shopify#2618) Add a Color Scheme setting for Menus-Header (Shopify#2622) Made mobile drawer full width by default-header (Shopify#2625) Allow multiple announcement bars in Header group (Shopify#2619) [Feat Product] Add rating styling sheet (Shopify#2620) Fix password page variables (Shopify#2607) Fix transform applied when it should not for sliders (Shopify#2606) Modify info string for gift card recipient checkbox (Shopify#2588) [3D lift animation] Raise hovered card above adjacent cards (Shopify#2589) Remove fallback color scheme info text (Shopify#2602) Fix CSS specifity issue to cancel animation for theme editor events (Shopify#2605) Remove settings daya for icon color (Shopify#2601) Follow ups for accessibility of announcements slider (Shopify#2580) ...
PR Summary:
Fix shadows cast by cards with 3D Lift enabled
Why are these changes introduced?
As noted over in #2451 (comment), there's a minor bug in the way shadows are cast by cards when the 3D Lift effect is active. The card appears to rise, but its shadow appears behind the adjacent cards. This breaks the illusion, making the effect less effective visually.
What approach did you take?
I added a slightly increased
z-index
to the parent container of cards and collage items.We usually apply hover effects to the
card-wrapper
class, but increasing the z-index there doesn't solve the issue. I had to reach one level higher in the DOM to get the card to actually appear lifted. Not totally ideal, but it's what worked. 🤷Visual impact on existing themes
Testing steps/scenarios
Demo links
Checklist