fix: remove flicker in AccordionItem (#9818) #14776

cesardlinx · 2023-10-04T14:24:49Z

Removes the flickering effect when using AccordionItem as a controlled component

Closes #9818

Changelog

Changed

I added a storybook example of the accordion as a controlled component

Removed

I removed an extra condition and a state that was the causing the problem

Testing / Reviewing

I run yarn test to verify that the Accordion related tests are still running

You can check the Accordion Controlled example in Storybook and verify that now the flickering is gone

Here you can see how it was working before:

And this is how it is working now:

netlify · 2023-10-04T14:30:03Z

✅ Deploy Preview for v11-carbon-react ready!

Name	Link
🔨 Latest commit	`36ec382`
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-react/deploys/656756763c1fdc00085440d3
😎 Deploy Preview	https://deploy-preview-14776--v11-carbon-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2023-10-04T14:32:45Z

✅ Deploy Preview for carbon-elements ready!

Name	Link
🔨 Latest commit	`b39e9f0`
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-elements/deploys/654c21fd57c1db0007418280
😎 Deploy Preview	https://deploy-preview-14776--carbon-elements.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

tw15egan · 2023-10-04T15:17:40Z

Still seeing a flicker when the Accordion is being closed - MacOS running Chrome latest

cesardlinx · 2023-10-04T17:07:15Z

Nice catch @tw15egan! You're absolutely right, I have updated the previous commit, this is how the tests are running

And this is how it's working now:

tw15egan · 2023-10-04T17:50:31Z

@cesardlinx unfortunately, this change removed the entire collapsing animation. I believe the animation itself may need to be reworked, and not rely on display: none since that seems to be causing the issue

cesardlinx · 2023-10-04T19:26:49Z

@tw15egan Yep! that's right. Let me elaborate more on why. Basically the main issue is that the collapsing animation is not actually "collapsing", instead it's "shrinking" as you can see here:

So, the hacky trick that somebody used is to mixed the shrinking with the "display: none" in order to create the "collapsing" effect like this:

And that's why you experience it like a small flickering when it collapses, because it's the combination of "shrinking" and then using "display: none". So what I did was to use only "display: none" in order to avoid the flickering.

tw15egan · 2023-10-04T19:29:12Z

@cesardlinx the preferred solution would be to retain the animation and remove the flicker. The animation itself may need to be reworked.

cesardlinx · 2023-10-04T19:31:00Z

@tw15egan I can rework the animation, but I don't think I can access it am I right?

tw15egan · 2023-10-04T19:32:44Z

Great, if you are interested in trying to fix the animation, it is located in packages/styles/scss/components/accordion.scss

cesardlinx · 2023-10-04T19:35:00Z

Nice! so cool, I thought that you were using an external package or something, that's great!! I'll fix it then :D

cesardlinx · 2023-10-04T23:06:51Z

@tw15egan Animation reworked! This is how it works now:

I'm getting some snapshot errors though as you can see here:

Should I update the snapshots? Please let me know

cesardlinx · 2023-10-10T21:02:13Z

Hey @tw15egan any updates on this? were you able to verify how the animation is working now?

tay1orjones

I don't see the flicker anymore but have a few concerns below.

packages/react/src/components/Accordion/AccordionItem.tsx

packages/styles/scss/components/accordion/_accordion.scss

cesardlinx · 2023-10-17T18:45:05Z

Hey @tay1orjones. I've addressed all the issues you mentioned earlier. I would greatly appreciate your feedback when you have the time. Please take a look at my comments on changing the markup. I explain why we should add a wrapper around the accordion content.

tay1orjones

Thanks for making all those changes! I'm sorry for such a delay in reviewing. This is looking good, I have just two small comments after re-reviewing.

tay1orjones · 2023-11-08T20:24:13Z

packages/react/src/components/Accordion/AccordionItem.tsx

+    const toggleElement = event.target;
+
+    let content = toggleElement.nextElementSibling;
+
+    // when toggle element is not the button
+    if (toggleElement.nodeName !== 'BUTTON') {
+      content = toggleElement.parentElement.nextElementSibling;
+    }


Gaining access to the content node this way feels brittle. Instead, could a ref be placed on the div containing the children, and then that contentRef.current.style be used in the maxBlockSize logic below?

tay1orjones · 2023-11-08T20:24:58Z

packages/react/src/components/Accordion/AccordionItem.tsx

+      content = toggleElement.parentElement.nextElementSibling;
+    }
+
+    if (content.style.maxBlockSize) {


Would it make more sense to use isOpen here?

tay1orjones · 2023-11-08T20:27:26Z

Also snapshots need to be updated, just run yarn test -u from the project root and push up the changes. Thanks!

Removes the flickering effect when using AccordionItem as a controlled component

tay1orjones

Thanks for addressing that feedback. I don't see the flicker in the storybook example. Removing the test story after a second approval and before merging would be good 👍

cesardlinx · 2023-11-27T21:24:31Z

Awesome! Thank you @tay1orjones for all the feedback, I will remove the test story after second approval 👍

andreancardona

looks good! @cesardlinx

cesardlinx · 2023-11-28T01:59:58Z

Thank you @andreancardona I have removed the test story as @tay1orjones suggested 🙂

carbon-automation · 2023-12-01T17:48:18Z

Hey there! v11.44.0 was just released that references this issue/PR.

…bon-design-system#14776) * fix: remove flicker in AccordionItem (carbon-design-system#9818) Removes the flickering effect when using AccordionItem as a controlled component * fix: rework accordion component animation (carbon-design-system#9818) * refactor: use ref hook for accordion content * test: update accordion component snapshots * docs: fix extra missing curly brace in .all-contributorsrc * docs: remove accordion controlled test story --------- Co-authored-by: Andrea N. Cardona <cardona.n.andrea@gmail.com> Co-authored-by: Alison Joseph <alison.joseph@us.ibm.com>

cesardlinx requested a review from a team as a code owner October 4, 2023 14:24

cesardlinx requested review from alisonjoseph and andreancardona October 4, 2023 14:24

cesardlinx mentioned this pull request Oct 4, 2023

[Bug]: Flicker in AccordionItem children #9818

Closed

2 tasks

cesardlinx force-pushed the issue_9818 branch from 4c6065f to 951827c Compare October 4, 2023 17:03

cesardlinx force-pushed the issue_9818 branch from 951827c to 33095fd Compare October 4, 2023 23:02

tay1orjones requested changes Oct 11, 2023

View reviewed changes

cesardlinx force-pushed the issue_9818 branch from 33095fd to 7c32a37 Compare October 12, 2023 03:48

alisonjoseph added the status: waiting for maintainer response 💬 label Oct 30, 2023

tay1orjones reviewed Nov 8, 2023

View reviewed changes

tay1orjones added status: ready for review 👀 and removed status: waiting for maintainer response 💬 labels Nov 8, 2023

cesardlinx added 4 commits November 8, 2023 18:50

fix: remove flicker in AccordionItem (carbon-design-system#9818)

de94fd3

Removes the flickering effect when using AccordionItem as a controlled component

fix: rework accordion component animation (carbon-design-system#9818)

c3c75bc

refactor: use ref hook for accordion content

0a8072f

test: update accordion component snapshots

0d30252

cesardlinx force-pushed the issue_9818 branch from 60fb9be to 0d30252 Compare November 8, 2023 23:53

cesardlinx and others added 2 commits November 8, 2023 19:04

Merge branch 'main' into issue_9818

b39e9f0

Merge branch 'main' into issue_9818

d739e75

andreancardona removed the status: ready for review 👀 label Nov 16, 2023

andreancardona requested a review from tay1orjones November 16, 2023 00:07

andreancardona added the status: one more review 👀 label Nov 16, 2023

cesardlinx added 2 commits November 17, 2023 20:10

docs: fix extra missing curly brace in .all-contributorsrc

de62cf1

Merge branch 'main' into issue_9818

2588f6f

tay1orjones added status: ready for review 👀 and removed status: one more review 👀 labels Nov 27, 2023

tay1orjones approved these changes Nov 27, 2023

View reviewed changes

tay1orjones added status: one more review 👀 and removed status: ready for review 👀 labels Nov 27, 2023

andreancardona approved these changes Nov 28, 2023

View reviewed changes

andreancardona added status: ready to merge 🎉 and removed status: one more review 👀 labels Nov 28, 2023

github-actions bot added this pull request to the merge queue Nov 28, 2023

docs: remove accordion controlled test story

131cf55

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 28, 2023

Merge branch 'main' into issue_9818

36ec382

tay1orjones added this pull request to the merge queue Nov 30, 2023

Merged via the queue into carbon-design-system:main with commit 448c0ef Nov 30, 2023
20 checks passed

jayolee mentioned this pull request Dec 7, 2023

[Bug]: open prop is ignored on AccordionItem #15347

Closed

2 tasks

joker23 mentioned this pull request Dec 13, 2023

[Bug]: AccordionItem - content height is not being set on initial open #15388

Closed

2 tasks

tay1orjones mentioned this pull request Jan 24, 2024

Provide per-package changelogs #15601

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove flicker in AccordionItem (#9818) #14776

fix: remove flicker in AccordionItem (#9818) #14776

cesardlinx commented Oct 4, 2023

netlify bot commented Oct 4, 2023 •

edited

Loading

netlify bot commented Oct 4, 2023 •

edited

Loading

tw15egan commented Oct 4, 2023

cesardlinx commented Oct 4, 2023 •

edited

Loading

tw15egan commented Oct 4, 2023

cesardlinx commented Oct 4, 2023

tw15egan commented Oct 4, 2023

cesardlinx commented Oct 4, 2023

tw15egan commented Oct 4, 2023

cesardlinx commented Oct 4, 2023 •

edited

Loading

cesardlinx commented Oct 4, 2023

cesardlinx commented Oct 10, 2023

tay1orjones left a comment

cesardlinx commented Oct 17, 2023

tay1orjones left a comment

tay1orjones Nov 8, 2023

tay1orjones Nov 8, 2023

tay1orjones commented Nov 8, 2023

tay1orjones left a comment

cesardlinx commented Nov 27, 2023

andreancardona left a comment

cesardlinx commented Nov 28, 2023

carbon-automation bot commented Dec 1, 2023

fix: remove flicker in AccordionItem (#9818) #14776

fix: remove flicker in AccordionItem (#9818) #14776

Conversation

cesardlinx commented Oct 4, 2023

Changelog

Testing / Reviewing

netlify bot commented Oct 4, 2023 • edited Loading

✅ Deploy Preview for v11-carbon-react ready!

netlify bot commented Oct 4, 2023 • edited Loading

✅ Deploy Preview for carbon-elements ready!

tw15egan commented Oct 4, 2023

cesardlinx commented Oct 4, 2023 • edited Loading

tw15egan commented Oct 4, 2023

cesardlinx commented Oct 4, 2023

tw15egan commented Oct 4, 2023

cesardlinx commented Oct 4, 2023

tw15egan commented Oct 4, 2023

cesardlinx commented Oct 4, 2023 • edited Loading

cesardlinx commented Oct 4, 2023

cesardlinx commented Oct 10, 2023

tay1orjones left a comment

Choose a reason for hiding this comment

cesardlinx commented Oct 17, 2023

tay1orjones left a comment

Choose a reason for hiding this comment

tay1orjones Nov 8, 2023

Choose a reason for hiding this comment

tay1orjones Nov 8, 2023

Choose a reason for hiding this comment

tay1orjones commented Nov 8, 2023

tay1orjones left a comment

Choose a reason for hiding this comment

cesardlinx commented Nov 27, 2023

andreancardona left a comment

Choose a reason for hiding this comment

cesardlinx commented Nov 28, 2023

carbon-automation bot commented Dec 1, 2023

netlify bot commented Oct 4, 2023 •

edited

Loading

netlify bot commented Oct 4, 2023 •

edited

Loading

cesardlinx commented Oct 4, 2023 •

edited

Loading

cesardlinx commented Oct 4, 2023 •

edited

Loading