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

Fix border card hover safari #1140

Merged
merged 5 commits into from
Jan 11, 2022
Merged

Fix border card hover safari #1140

merged 5 commits into from
Jan 11, 2022

Conversation

sofiamatulis
Copy link
Contributor

Why are these changes introduced?

Fixes #1121 .

What approach did you take?

Updated the z-index for the card and the media. The reason this was happening is because when we apply transform or opacity (which we are for the animation), the order doesn't stay the same even with position: relative. So the solution is to set the index. More here: https://coder-coder.com/z-index-isnt-working/

Other considerations

Demo links

Test on safari:

Checklist

Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

Good find! I think we just need to fiddle with it a little more to ensure we don't cause shadow overlaps. Hopefully it's straightforward adjustment.

@ghost ghost added the cla-needed label Jan 10, 2022
@sofiamatulis
Copy link
Contributor Author

I have opted to fix this with z-index since it's a cleaner/more intuitive solution, but I found a thread about this same bug here: https://discourse.webflow.com/t/safari-not-hiding-overflow-on-rounded-corner-divs/55060

Another solution for Safari seems to be to apply:

 -webkit-mask-image: -webkit-radial-gradient(white, black);

kmeleta
kmeleta previously approved these changes Jan 10, 2022
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

Nice! glad the z-index fix could still work out. Tested the fix in safari and looked for shadow/z-related regressions and didn't notice any issues.

@@ -37,6 +37,7 @@

.card .card__inner .card__media {
overflow: hidden;
z-index: 0;
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 we try to avoid comments in code, but I'm tempted to include a short one on this line that makes it known it's needed for a safari bug. I could see it appearing as though it's not doing anything and getting removed in the future. Not sure how everyone feels about that..

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be a good idea - CSS isn't very self-explanatory.

Copy link
Contributor

Choose a reason for hiding this comment

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

I figure if it was a cross-browser bug or even a chrome bug it'd be fine, but I think it'll get overlooked in safari, especially being an on-hover issue.

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 point! Added

tyleralsbury
tyleralsbury previously approved these changes Jan 10, 2022
Copy link
Contributor

@tyleralsbury tyleralsbury left a comment

Choose a reason for hiding this comment

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

Approved - Maybe add a comment like Ken mentioned.

Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

:shipit:

@sofiamatulis sofiamatulis merged commit 26cf28f into main Jan 11, 2022
@sofiamatulis sofiamatulis deleted the safari-card-border branch January 11, 2022 15:06
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Safari] Second image on hover doesn't follow corner radius
3 participants