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

[3D lift animation] Raise hovered card above adjacent cards #2589

Merged
merged 9 commits into from
May 2, 2023
Merged

[3D lift animation] Raise hovered card above adjacent cards #2589

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2b1678b
Raise hovered card above adjacent cards.
kjellr Apr 28, 2023
777fa46
Target more specific classes.
kjellr Apr 28, 2023
9131f00
Merge branch 'main' into fix-hover-state-for-3d-lift
ludoboludo Apr 28, 2023
fc40e5e
Increase the z-index so that complementary slides work.
kjellr Apr 28, 2023
0cdd592
Only change z-index for hovered articles.
kjellr Apr 28, 2023
3ac04c4
Merge branch 'main' into fix-hover-state-for-3d-lift
kjellr May 1, 2023
a427f0e
Remove unnecessary z-index rule.
kjellr May 1, 2023
459227f
Merge branch 'main' into fix-hover-state-for-3d-lift
kjellr May 1, 2023
0a95ba2
Ensure cards don't overlap search popover.
kjellr May 1, 2023
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
5 changes: 5 additions & 0 deletions assets/base.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3353,6 +3353,11 @@ details-disclosure > details {
transform: rotate(0.5deg); /* Less intense rotation for collage items. */
}

.animate--hover-3d-lift .grid__item:hover,
Copy link
Contributor

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

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 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 .collage__item:hover {
z-index: 1; /* Make sure the hovered card is the topmost card. */
}
kjellr marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this would be a good followup. There's a bug with complementary products where the shadow doesn't extend out of its container:

Screenshot 2023-04-28 at 4 13 05 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't target complementary product cards as they're not using a grid__item class:

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 catch. This should be fixed with the combo of 777fa46 and fc40e5e. For the latter, I had to increase the z-index from 1 to 2 to get it to work here. I'm not entirely sure why — 1 worked great in every other context. 🤷

Copy link
Contributor

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.

Copy link
Contributor Author

@kjellr kjellr May 1, 2023 •
edited
Loading

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.


.animate--hover-3d-lift .card-wrapper .card--shape.card--standard:not(.card--text) .card__inner {
box-shadow: none;
transition: transform var(--duration-long) ease, filter var(--duration-long) ease;
Expand Down