fix(#3625): Accordion Refinements by willcodeforcoffee · Pull Request #3766 · GovAlta/ui-components

willcodeforcoffee · 2026-04-06T20:53:05Z

Before (the change)

Accordion is all unrefined and gross. 🤢

After (the change)

Fixed all discrepancies from issue #3625.

Large heading Accordion is 64px tall
Small heading Accordion is 48px tall
After size adjustments above:
- the small heading text is vertically centered for
  - primary text
  - secondary text
- large heading secondary text is vertically centered
Ensure other slotted elements are still vertically centered after update
Add motion tokens to animate the following:
- "hover" - Background transform from white to grey use --goa-motion-duration-short-2
- "open" - Show accordion content area use --goa-motion-curve-expressive and --goa-motionDuration-short-3
- "close" - Hide accordion content area, use --goa-motion-curve-expressive-exit and --goa-motionDuration-short-3

Make sure that you've checked the boxes below before you submit the PR

I have read and followed the setup steps
I have created necessary unit tests
I have tested the functionality in both React and Angular.

Steps needed to test

Make sure you check out the eric/3625-accordion-refinements branch from design-tokens and update package.json devDependencies to use it:

"@abgov/design-tokens": "file://../design-tokens",
"@abgov/design-tokens-v2": "npm:@abgov/design-tokens@2.2.1",

Run npm i after making the change.

This PR will close Issue #3625

Spark450 · 2026-04-08T23:17:54Z

    background-color: var(--goa-accordion-color-bg-heading-hover);
    border: var(--goa-accordion-border-hover, var(--goa-accordion-border));
    color: var(--goa-accordion-color-heading-hover);
+    transition: var(--goa-motion-duration-medium-1) background-color


Lets use --goa-motionDuration-short-2 to align more closely with the "short-2" we are using for other similar elements.

netlify · 2026-04-09T21:29:42Z

✅ Deploy Preview for benji-docs-previews ready!

Name	Link
🔨 Latest commit	`f06e851`
🔍 Latest deploy log	https://app.netlify.com/projects/benji-docs-previews/deploys/69ea34b3180f3b000834424b
😎 Deploy Preview	https://deploy-preview-3766--benji-docs-previews.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 project configuration.

bdfranck

I tested the changes in the latest browsers...

Safari (Latest)

✅ The sizes look good
✅ The animation works as expected

Chrome (Latest)

✅ The sizes look good
✅ The animation works as expected

Chrome (Latest)

✅ The sizes look good
✅ The animation works as expected

Looks good! I left a couple minor comments.

bdfranck · 2026-04-13T22:05:27Z

    background-color: var(--goa-accordion-color-bg-heading-hover);
    border: var(--goa-accordion-border-hover, var(--goa-accordion-border));
    color: var(--goa-accordion-color-heading-hover);
+    transition: var(--goa-motion-duration-short-2) background-color


Add the transition to the base summary selector on line 338 so the transitions both works on hover and blur.

bdfranck · 2026-04-13T22:40:34Z

  }

-  .heading {
+  .heading-small {


I'm still seeing the small heading is 50px instead of 48px. Is the dependency because of the borders?

🤔 Hmm, you're right! I must not have included the borders in my calculations.

@bdfranck I was able to fix this, you just need to get the latest of design-tokens first.

bdfranck · 2026-04-13T22:41:15Z

+  }
+
+  .heading-medium {
+    line-height: 2rem;


I'm still seeing the medium heading is 66px instead of 64px. Is the dependency because of the borders?

bdfranck · 2026-04-14T14:51:52Z

+    event.stopPropagation();
+  }
+
+  function onAccordionToggle(target: EventHandler) {


When I slowed down the animation in the Chrome Dev Tools Animation panel, I noticed that it wasn't including the component's bottom border. Is there an easy way to address this? 🤔

@bdfranck 🤔 🤔 🤔 I believe this is a consequence of animating it using height attributes..? Do you have a suggestion? Maybe if I animate the height of an internal div instead? I can try that and see what I can do...

After team discussion, we'll leave this animation as is.

willcodeforcoffee · 2026-04-15T23:39:39Z

Addressed feedback:

moved the transition to the base summary
fixed the sizes of the headings
fixed animation speed to --goa-motion-duration-short-3 instead of short-2
added warning message and fallback so the accordion still works if the wrong version of design-tokens is included
changed EventHandler type to Event
open and aria-expanded attributes work correctly
removed commented CSS

@bdfranck @vanessatran-ddi ready for a re-review! 🙏

bdfranck

I looked at the latest changes with the tokens from GovAlta/design-tokens#145...

✅ The small heading height is 48px and the medium heading height is 64px
✅ I see aria-expanded is true when the accordion is open
✅ The animation works as expected

Looks good to me! 👍

willcodeforcoffee · 2026-04-21T01:57:12Z

Finished addressing comments:

open instead of isOpen
refactored the CSS time parsing

Thanks again for the thoughtful feedback @vanessatran-ddi .

vanessatran-ddi

Looks good!

willcodeforcoffee · 2026-04-27T15:56:38Z

@ArakTaiRoth This is ready to merge! 🙏

ArakTaiRoth · 2026-04-27T17:43:52Z

+    // If the animation is cancelled, isClosing variable is set to false
+    _animation.oncancel = () => (_isClosing = false);
+
+    open = "false";


Because we're setting open to false here, before onAnimationFinish actually finishes. This causes the content to disappear before the Accordion actually closes.

jam.dev

@ArakTaiRoth Good catch, I wasn't looking close enough at the animation afterwards!
This is now fixed. I didn't see any issues or regressions.

github-actions · 2026-04-27T19:57:32Z

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-04-27 22:46 UTC

github-actions · 2026-04-27T19:57:48Z

Preview removed

All preview folders cleaned from gh-pages branch.

ArakTaiRoth · 2026-04-27T20:11:42Z

+
+  function onAnimationFinish(isAccordionOpen: boolean) {
+    // Set the open attribute based on the parameter
+    _detailsEl.open = isAccordionOpen;


I'm really sorry about this. But your previous fix for my last issue introduced a new issue. Now because open isn't being set to false anymore in shrink(), aria-expanded isn't being updated properly, so now when you close an accordion, aria-expanded still reads as true, you can test this with any accordion in your PR example.

I would suggest maybe moving the open property handling to onAnimationFinish instead of handling it in shrink() or expand(), and just changing what the value is everytime it's run, open = isAccordionOpen ? "true" : "false";.

You have this line here that handles _detailsEl.open, but that doesn't modify open, which is what's deriving isOpen, which is what's used for aria-expanded.

@ArakTaiRoth another good catch! 🤦 Sorry I missed that one, too. 😞

ArakTaiRoth · 2026-04-27T20:59:35Z

+  }
+
+  function onAnimationFinish(isAccordionOpen: boolean) {
+    console.log("onfinish");


Remove this

chrisolsen · 2026-04-27T22:48:09Z

🎉 This PR is included in version 2.0.0-dev.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

chrisolsen · 2026-04-27T22:48:35Z

🎉 This PR is included in version 7.0.0-dev.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

chrisolsen · 2026-04-28T18:54:22Z

🎉 This PR is included in version 5.0.0-dev.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

chrisolsen · 2026-05-04T21:31:16Z

🎉 This PR is included in version 2.0.0-dev.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

willcodeforcoffee self-assigned this Apr 6, 2026

willcodeforcoffee added Accordion 2.0 P3 Priority 3 (nice to have): Valuable, but safe to release after launch. labels Apr 6, 2026

willcodeforcoffee force-pushed the eric/3625-accordion-refinements branch from 25f7b18 to acd6c01 Compare April 6, 2026 21:47

Spark450 reviewed Apr 8, 2026

View reviewed changes

willcodeforcoffee force-pushed the eric/3625-accordion-refinements branch from 7415d38 to 6134646 Compare April 9, 2026 02:43

willcodeforcoffee linked an issue Apr 9, 2026 that may be closed by this pull request

Accordion refinement #3625

Closed

willcodeforcoffee force-pushed the eric/3625-accordion-refinements branch from 6134646 to aab8bdc Compare April 9, 2026 03:08

willcodeforcoffee force-pushed the eric/3625-accordion-refinements branch 2 times, most recently from 5c66489 to 6053134 Compare April 10, 2026 22:25

willcodeforcoffee marked this pull request as ready for review April 10, 2026 22:48

willcodeforcoffee requested review from bdfranck and vanessatran-ddi April 10, 2026 22:49

bdfranck reviewed Apr 14, 2026

View reviewed changes