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

Setting display:flex on accordion content creates double animation #2832

Open
BernieSumption opened this issue Apr 11, 2024 · 2 comments
Open

Comments

@BernieSumption
Copy link

BernieSumption commented Apr 11, 2024

Bug report

Current Behavior

The default display for Accordion.Content is block. Setting it to flex causes a double animation when closing a section.

Expected behavior

The animation should not be affected by using display:flex

Reproducible example

Open / close the sections on this codesandbox: https://codesandbox.io/s/2r30e

Suggested solution

If this is technically not possible / too difficult to remove the double animation, a warning in the console would prevent a lot of debugging.

Your environment

Software Name(s) Version
Radix Package(s) @radix-ui/react-accordion 1.1.2
React 18.2.0
Browser Safari / Chrome 17.2.1 / 123.0.6312.107
Assistive tech none
Node n/a
npm/yarn codesandbox
Operating System macOS 14.2.1 (23C71)
@joseph0926
Copy link

joseph0926 commented Sep 4, 2024

(I have an awkward sentence using the translator)

The problem seems to be a conflict between automatically calculating heights in flex and in radix.

const rect = node.getBoundingClientRect();
heightRef.current = rect.height;
widthRef.current = rect.width;

As you can see from the code above, we are dynamically calculating the height inside the radix

However, the flex layout automatically calculates and places the height dynamically based on the size of the child elements. Therefore, the dynamically calculated height in Radix and the automatically calculated height in flex layout will conflict with each other, resulting in the height being calculated twice and animated twice as a result

So, the solution is as follows

const slideDown = keyframes({
  from: { maxHeight: 0 },
  to: { maxHeight: "var(--radix-accordion-content-height)" },
});

const slideUp = keyframes({
  from: { maxHeight: "var(--radix-accordion-content-height)" },
  to: { maxHeight: 0 },
});

max-height(maxHeight) seems to solve the problem

max-height doesn't conflict with the automatic height calculation of flex layouts because the browser doesn't need to dynamically calculate the size during animation.
The height attribute is used to animate the exact height, but it is prone to conflicts because dynamic values like auto are not supported.
On the other hand, max-height is calculated with a fixed value, so it animates smoothly without conflicts.

Of course, I've only tested a very small part of this, so I'd love to hear back from the “radix” team

@hoop71
Copy link
Contributor

hoop71 commented Sep 18, 2024

I am struggling with the same bug @joseph0926! It also is affected with display: grid. Thanks for posting a solution. I will keep watching. Am also happy to contribute if needed. Thanks.

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

No branches or pull requests

3 participants