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: display none has negative impact on svgs used inside a collapse #2311

Closed
wants to merge 3 commits into from

Conversation

Lelith
Copy link
Collaborator

@Lelith Lelith commented Oct 31, 2022

Purpose of PR

This is a side-effect fix.

I have multiple collapsable containers using this collapse animation. Inside of these containers i have a colorful svg icon with a gradient in several of them.

Every time the first container that has the icon gets collapsed, the gradient disappears. This is a known chromium bug since almost 10 years now The issue exists not only in chrome but also in firefox (it was also in webkit, but safari fixed it at some point)

What happens is, that the reference all of the svg files have to the gradient (via id) always points to its first defintion. When this first svg is now set to display:none this defintion stops to exist for all following instances of the same svg in the page.

The simplest to achieve fix is: do not use display:none but visibility:hidden to hide the content.

If there are no objections or other reasons why display:none is the important rule, please let me know and we can think about other solutions.

Maybe its also worth checking again the proposal here #2043

@Lelith Lelith requested a review from fabe as a code owner October 31, 2022 16:17
@vercel
Copy link

vercel bot commented Oct 31, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
forma-36 ✅ Ready (Inspect) Visit Preview Nov 1, 2022 at 1:21PM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Oct 31, 2022

🦋 Changeset detected

Latest commit: 69c2498

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 33 packages
Name Type
@contentful/f36-collapse Patch
@contentful/f36-accordion Patch
@contentful/f36-asset Patch
@contentful/f36-autocomplete Patch
@contentful/f36-badge Patch
@contentful/f36-button Patch
@contentful/f36-card Patch
@contentful/f36-copybutton Patch
@contentful/f36-datepicker Patch
@contentful/f36-datetime Patch
@contentful/f36-drag-handle Patch
@contentful/f36-entity-list Patch
@contentful/f36-forms Patch
@contentful/f36-icon Patch
@contentful/f36-icons Patch
@contentful/f36-list Patch
@contentful/f36-menu Patch
@contentful/f36-modal Patch
@contentful/f36-note Patch
@contentful/f36-notification Patch
@contentful/f36-pagination Patch
@contentful/f36-pill Patch
@contentful/f36-popover Patch
@contentful/f36-skeleton Patch
@contentful/f36-spinner Patch
@contentful/f36-table Patch
@contentful/f36-tabs Patch
@contentful/f36-text-link Patch
@contentful/f36-tooltip Patch
@contentful/f36-typography Patch
@contentful/f36-utils Patch
@contentful/f36-core Patch
@contentful/f36-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link

github-actions bot commented Oct 31, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
CommonJS 120.83 KB (+0.01% 🔺) 2.5 s (+0.01% 🔺) 413 ms (-2.5% 🔽) 2.9 s
Module 118.63 KB (+0.01% 🔺) 2.4 s (+0.01% 🔺) 333 ms (+16.87% 🔺) 2.8 s

Copy link
Collaborator

@denkristoffer denkristoffer left a comment

Choose a reason for hiding this comment

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

I wonder if this could break use of useEffect in children components 🤔 Any idea?

@denkristoffer
Copy link
Collaborator

Every time the first container that has the icon gets collapsed, the gradient disappears. This is a known chromium bug since almost 10 years now The issue exists not only in chrome but also in firefox (it was also in webkit, but safari fixed it at some point)

Do you have a reproduction of this I can try?

@Lelith
Copy link
Collaborator Author

Lelith commented Nov 1, 2022

@denkristoffer here is a super quick codepen: https://codepen.io/Lelith/pen/rNKxJBG
if you change the css from display:none to visibility:hidden the svg looks good again.

I would not see why it would have a side effect for useEffect, as the css change is still programmatically via the toggle, only with different content. So a rerender is clearly forced.

@denkristoffer
Copy link
Collaborator

I figured the switch might affect the DOM but it seems to be fine.

@denkristoffer
Copy link
Collaborator

Not sure what's going on with Chromatic, let me try rerunning CI from scratch.

@Lelith
Copy link
Collaborator Author

Lelith commented Nov 2, 2022

@denkristoffer i saw that too, and locally it got fixed once i rebuilt the packages. it seems to be stuck in a inbetween state, like updating the css rules but not the javascript (or vice versa?)

@Lelith
Copy link
Collaborator Author

Lelith commented Nov 3, 2022

so i tried it in our app, but it breaks the behaviour of the dragable again, so will close it

@Lelith Lelith closed this Nov 3, 2022
@Lelith Lelith deleted the fix/svg_issues_with_display_none branch February 15, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants