Skip to content

feat(accordion): add direct actions #4020

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

Merged
merged 9 commits into from
Jul 29, 2025

Conversation

rise-erpelding
Copy link
Collaborator

@rise-erpelding rise-erpelding commented Jul 9, 2025

Description

Gives the accordion component a minor update to include direct actions, which may consist of a quiet action button, switch, or both. Direct action items are vertically centered within the heading's first line of text for all sizes and densities, and maintain their own individual key focus states.

Also updates the default content in the accordion to better align with the default width examples -- the content we had before works well for an accordion with custom width (and will stay for that example in the docs) but otherwise wraps too much.

CSS-1204

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

  1. Open the PR preview or check out the branch locally and check out the accordion default story. (@cdransf)
  2. Add direct actions (switch, action button, and both together) at each t-shirt size and density to confirm correct vertical (centered within heading's first line of text) placement:
    • Compact density (S, M, L, XL) (@cdransf)
    • Regular density (S, M, L, XL) (@cdransf)
    • Spacious density (S, M, L, XL) (@cdransf)
  3. Confirm that direct actions' horizontal placement is correct:
    • Direct actions' start position does seem to indicate that it's 8px/spacing-100 from the item button text (.spectrum-Accordion-itemHeader), but inspecting closer this seems to be within the action button (8px from the edge of the button to what looks like maybe the center of the stroke on the icon?). Therefore, I didn't add any spacing but am questioning if this is the right call. (@cdransf)
    • Spacing between the direct actions, if both switch and action button are present, should be spacing-100/8px. This wasn't specifically noted in the token spec, but can be confirmed by checking the spacing in the token specs Figma, it's noted in the Jira ticket as well. (@cdransf)
    • Spacing at the end of direct actions should align with the accordion item's inline-end spacing (same as .spectrum-Accordion-itemHeader's inline-end spacing, using the accordion-edge-to-content-area tokens) (@cdransf)
  4. Within .spectrum-Accordion-itemHeading, the direct actions container has been added as a sibling to the button (.spectrum-Accordion-itemHeader) so that the direct actions take up as much space as they need, and the button takes up any remaining space, and wraps when needed. Are there any issues with focus states, text wrapping, or anything else related to adding the direct actions into an item heading?
    • Focus states (@cdransf)
    • Text wrapping (very happy to have input on this, with the default item width, there's not a lot of space for the heading text so there's a lot of breaking right now)
    • Other issues (@cdransf)
  5. Check that testing and documentation accurately cover these changes.
    • Docs page (@cdransf)
    • Testing grid (tests append some direct actions items to the existing grids so that we don't need to add separate direct actions cases for each size and density) (@cdransf)

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

image

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@rise-erpelding rise-erpelding self-assigned this Jul 9, 2025
@rise-erpelding rise-erpelding added wip This is a work in progress, don't judge. S2 Spectrum 2 labels Jul 9, 2025
Copy link

changeset-bot bot commented Jul 9, 2025

🦋 Changeset detected

Latest commit: 192bbd3

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

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

Copy link
Contributor

github-actions bot commented Jul 9, 2025

File metrics

Summary

Total size: 1.44 MB*

Package Size Minified Gzipped
accordion 21.44 KB 20.59 KB 2.64 KB

accordion

Filename Head Minified Gzipped Compared to base
index.css 21.44 KB 20.59 KB 2.64 KB 🔴 ⬆ 1.48 KB
metadata.json 11.47 KB - - 🔴 ⬆ 0.62 KB
* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

Copy link
Contributor

github-actions bot commented Jul 9, 2025

📚 Branch preview

PR #4020 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-4020/index.html.

@rise-erpelding rise-erpelding force-pushed the rise-erpelding/accordion-followups branch 5 times, most recently from ec8d50d to 4480c6d Compare July 11, 2025 15:46
Comment on lines +77 to +83
/* Calculated vertical spacing for action button and switch to center them within the accordion item */
--spectrum-accordion-item-direct-actions-vertical-spacing: calc(
(var(--mod-accordion-item-min-block-size, var(--spectrum-accordion-item-min-block-size)) -
var(--mod-accordion-item-direct-actions-height, var(--spectrum-accordion-item-direct-actions-height))) / 2
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did I miss a better way to do vertical spacing for this situation? I was finding that this got pretty complex, pretty fast:

  • An align-items: center on the .spectrum-Accordion-itemDirectActions isn't ideal when the text wraps because the designs show that we want direct actions to align with the first line of text, instead of all of them.
  • Accordion items have a min-height of component-height-75/component-height-100/component-height-200/component-height-300, same with the direct actions, but we don't usually see that min-height in accordion (except for compact) because of the calculations for spacing and font-size * line-height making it larger

@rise-erpelding rise-erpelding force-pushed the rise-erpelding/accordion-followups branch from 4480c6d to d4138b1 Compare July 14, 2025 21:18
@rise-erpelding rise-erpelding added ready-for-review and removed wip This is a work in progress, don't judge. labels Jul 14, 2025
@rise-erpelding rise-erpelding marked this pull request as ready for review July 14, 2025 23:31
@rise-erpelding rise-erpelding added the size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. label Jul 15, 2025
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

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

Thanks for getting this into accordion!

Separately, I want to look into the align-items: flex-start in the switch component. To me, in this specific case, the action button icon and the switch don't look center-aligned to each other, but maybe that's ok. Inspecting Figma, there's 6px of space around the action button icon, but 8px of space around the switch, so it feels like there's an accommodation of that flex-start. I'm not sure this is the PR to investigate though.


& .spectrum-ActionButton,
& .spectrum-Switch {
margin-block-start: var(--mod-accordion-item-direct-actions-vertical-spacing, var(--spectrum-accordion-item-direct-actions-vertical-spacing));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although I'm not sure I have a way to avoid this calc (I think it looks the best, honestly), I wonder if we can slap the block-size on that itemDirectActions container instead, to keep it from growing.

If we put:

margin-block-start: var(--mod-accordion-item-direct-actions-vertical-spacing, var(--spectrum-accordion-item-direct-actions-vertical-spacing));
block-size: var(--spectrum-accordion-item-direct-actions-height);

on .spectrum-Accordion-itemDirectActions, I think we can remove this nested block with the action button and switch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved the margins up to .spectrum-Accordion-itemDirectActions, it needs both top and bottom for the compact XL variant (otherwise the action button isn't centered). It didn't seem like block-size was needed with that but maybe I missed something?

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt Jul 23, 2025

Choose a reason for hiding this comment

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

Ah ok. So the margin-block-* centers the action button icon with the text, correct? I was misunderstanding I think before. One thing I noticed with this, was that the action button then expands past the parent accordion item (because I think the sized action buttons are taller than the sized accordion item buttons). Is that ok, do you think?

Screenshot 2025-07-23 at 10 26 18 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it 100% bugs me too that the action button doesn't fit within the accordion item and that the itemDirectActions container is bigger than the accordion item, but I think this is only for XL compact (I guess technically for large compact we also end up with a negative margin but I think the browser might handle that as 0 since it's fractional?).

I'd love to get more design feedback on this one at some point. Do we think now is the better time to do that? One reason I can think of to hold back is that I'd thought the implementation of Adobe Clean Spectrum was going to help resolve some of our line height problems, which give us component height problems in certain components. Josh made that calc that we use here in accordion to set --spectrum-accordion-item-min-block-size to either the --spectrum-component-height-xx tokens or the sum of the top and bottom spacing and font size times line height. My understanding was that if the font's rolled out as proposed, the tokens would set font-size, line-height, and vertical-padding in a way that will always add up to the corresponding component height.

I'm wondering if we get that all resolved, if that will change anything related to the way the compact accordion item is spaced. It might not. Thoughts?

I'm also wondering if once design has more concrete guidance in place if they might decide to recommend that direct actions and XL accordions shouldn't coexist because it doesn't leave enough space for the heading text?

@rise-erpelding rise-erpelding force-pushed the rise-erpelding/accordion-followups branch from d4138b1 to b519e3f Compare July 21, 2025 18:13
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

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

I think I only have one real suggestion in adding the disabled arg! Nice work on this- for something that seems to straightforward, that calc to align the action button/switch is so complicated.

A couple of other things I noticed this time around:

  • I noticed that the accordion-top-to-text-compact-medium token is 8px, so eventually design will have to update that to 5px like the spec shows in Figma. I'm assuming 5px is correct, because 8px makes the text sit way too low to my eyes. The matching bottom-to-text is 5px as well. Are they aware that it's an incorrect value, or is that intentional?
Screenshot 2025-07-23 at 1 01 29 PM Screenshot 2025-07-23 at 1 03 48 PM
  • I saw your question about the space between the itemHeader text and the direct actions container being spacing-100. I also think it's odd that the measurement is to the action button icon. I'm not sure if we could do some sort of clamp(), but that really only gets the space closer to the desired 8px, it doesn't actually address the problem. I had something like:
.spectrum-Accordion-itemHeading:has(.spectrum-Accordion-itemDirectActions) .spectrum-Accordion-itemHeader {
    padding-inline-end: clamp(5px, calc(var(--mod-accordion-edge-to-content-area, var(--spectrum-accordion-edge-to-content-area)) - var(--spectrum-spacing-100)), var(--spectrum-spacing-100));
}

...and I picked 5px as the min, since I saw the action button padding was 3px. But I don't know how we account for the changes in the action button padding when the t-shirt sizing changes. I wonder if at the very least we can account for the spacing-100 in the variant without inline padding. Right now, it looks like all of the padding around itemHeader also goes away, instead of just the outermost padding. Am I understanding that story right? There should technically be some sort of spacing there right?

Screen.Recording.2025-07-23.at.1.09.26.PM.mov

@rise-erpelding
Copy link
Collaborator Author

I noticed that the accordion-top-to-text-compact-medium token is 8px, so eventually design will have to update that to 5px like the spec shows in Figma. I'm assuming 5px is correct, because 8px makes the text sit way too low to my eyes. The matching bottom-to-text is 5px as well. Are they aware that it's an incorrect value, or is that intentional?

Yep, I noticed it too, when I'd originally asked I didn't notice that it was done only for the regular density and not the compact/spacious. It looks like it was addressed in 13.12.0, so that hopefully will just work once we're taking in that version of tokens. We're a bit behind at the moment but hopefully we can address this in #4019?

Still looking into some of the other issues!

@rise-erpelding rise-erpelding force-pushed the rise-erpelding/accordion-followups branch from b519e3f to 964b0cd Compare July 24, 2025 00:18
@@ -13,7 +13,7 @@

.spectrum-Accordion {
/* Layout and spacing */
--spectrum-accordion-item-minimum-height: var(--spectrum-component-height-100);
--spectrum-accordion-item-minimum-height: var(--spectrum-component-height-200);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I was inspecting direct actions in Figma more, I also realized that our minimum heights don't really match what's represented on the spec.

There's a section in the token spec for component height (minimum height, they have it labeled as "Spacing (top/bottom edge to label)", previously we were using this as the minimum height for the regular densities, but I don't think that's correct. What I saw in Figma was that these were the minimum heights for the compact variant, then from there they get larger, like this:

Compact Regular Spacious
S 24px component-height-75 32px component-height-100 40px component-height-200
M 32px component-height-100 40px component-height-200 48px component-height-300
L 40px component-height-200 48px component-height-300 56px component-height-400
XL 48px component-height-300 56px component-height-400 64px component-height-500

I've updated these minimum heights to match. Would we feel better if I verified this with design though? Making this change means that the --spectrum-accordion-item-min-block-size that takes into account the max of that min height vs. the paddings + line height * font size will more frequently use the component-height tokens rather than the calc.

The other thing I noticed in my Figma deep-dive is that the direct actions action button has 0px of spacing from the top/bottom of the accordion item for all sizes of compact variants, 4px of spacing for regular variants, and 8px of spacing for spacious variants.

Updating the minimum heights here brings us way closer to those numbers for almost all of the size/density variants (I think regular small and spacious small are the exceptions among the English/desktop components). And then if we end up with accordion items that need more space based on the line height calculation (for regular small, spacious small, and I think almost all of the CJK variants), we still have vertically centered actions.

Happy to hear thoughts here!

Copy link
Member

Choose a reason for hiding this comment

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

If they're consistent with the values in Figma I'm all for updating them here. ✨

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt Jul 24, 2025

Choose a reason for hiding this comment

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

@rise-erpelding Because we updated the min-heights of the items, do you think there is any way for us to center the text now? Like you said, now a lot of the min-heights are larger than that calc that was taking into account the paddings and the font * line-height. But now, the paddings and font * line-height doesn't "matter" as much.

I noticed the titles are hovering at the tops of their item containers (with the correct paddings), but should they maybe be centered now? 🤷‍♀️

Screenshot 2025-07-24 at 4 39 25 PM

We could totally make this a new PR to investigate. I also noticed that the button doesn't really align all the way either. I'm assuming the gray hovered background should extend until the next border.

Screenshot 2025-07-24 at 4 58 15 PM

margin-inline-end: var(--mod-accordion-edge-to-content-area, var(--spectrum-accordion-edge-to-content-area));
display: inline-flex;
gap: var(--mod-accordion-item-direct-actions-spacing, var(--spectrum-accordion-item-direct-actions-spacing));

/* margin needs to be set on top and bottom to keep compact XL items vertically centered and prevent them from growing vertically */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I still think top and bottom margin being set is a good idea although it is probably less necessary now. I removed the comment since it no longer applies.

Copy link
Member

@cdransf cdransf left a comment

Choose a reason for hiding this comment

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

Looks great! ✨

@@ -13,7 +13,7 @@

.spectrum-Accordion {
/* Layout and spacing */
--spectrum-accordion-item-minimum-height: var(--spectrum-component-height-100);
--spectrum-accordion-item-minimum-height: var(--spectrum-component-height-200);
Copy link
Member

Choose a reason for hiding this comment

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

If they're consistent with the values in Figma I'm all for updating them here. ✨

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

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

I'm approving! I think the direct actions part of the PR is good to go, at least for now, specifically going into spectrum-two. We're beginning to uncover some other issues that we could definitely dive into, but I think we can merge this, particularly as it relates to getting this new feature into accordion! ✨

Thanks for such wonderful detective work!

@@ -13,7 +13,7 @@

.spectrum-Accordion {
/* Layout and spacing */
--spectrum-accordion-item-minimum-height: var(--spectrum-component-height-100);
--spectrum-accordion-item-minimum-height: var(--spectrum-component-height-200);
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt Jul 24, 2025

Choose a reason for hiding this comment

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

@rise-erpelding Because we updated the min-heights of the items, do you think there is any way for us to center the text now? Like you said, now a lot of the min-heights are larger than that calc that was taking into account the paddings and the font * line-height. But now, the paddings and font * line-height doesn't "matter" as much.

I noticed the titles are hovering at the tops of their item containers (with the correct paddings), but should they maybe be centered now? 🤷‍♀️

Screenshot 2025-07-24 at 4 39 25 PM

We could totally make this a new PR to investigate. I also noticed that the button doesn't really align all the way either. I'm assuming the gray hovered background should extend until the next border.

Screenshot 2025-07-24 at 4 58 15 PM

@rise-erpelding rise-erpelding force-pushed the rise-erpelding/accordion-followups branch 2 times, most recently from cd9e214 to 5cfcb4f Compare July 29, 2025 01:26
@rise-erpelding
Copy link
Collaborator Author

rise-erpelding commented Jul 29, 2025

Oof, really great callout on this one @marissahuysentruyt. Checking further it looks like design's Text stack for accordion items has the same problem and also does not reach to the bottom of the accordion item. However, it's less noticeable there because the hover state is on the whole accordion item, whereas ours is just on the button element inside the item.

I would agree that it feels like a separate ticket, but I think we can avoid the bottom gapping issue for now by reverting back to what was here before I adjusted the min height custom properties, and that doesn't cause any unwanted side effects... does that seem right?

Later on, we can consult more with design about what the intended minimum height is and the inconsistency between the text stack height and the item height. If moving to Adobe Clean Spectrum is in the near future, I wonder if it would be deemed worth fixing?

One other oddity I discovered that I'm noting here, if you hover on the accordion item with only a switch (no action button), there's no space between the hover background and the switch, in contrast to what you see when you hover the accordion item that's directly next to an action button. In Figma, if I hover the accordion item with a switch in place, it hovers over the entire item around the switch, which doesn't seem correct. We could probably figure out something on our own if needed (we may be able to use margin instead of padding), but it feels like it might be just as well to ask design.

image

- updates test cases in order to use shorter content that fits better
with the default width of accordion items
- adds direct actions feature to storybook, template, and css
- use template to ensure that disabled accordion items also have
disabled direct actions
- maintain padding-inline end on accordion item headers with direct
actions
@rise-erpelding rise-erpelding force-pushed the rise-erpelding/accordion-followups branch from 5cfcb4f to 192bbd3 Compare July 29, 2025 17:00
@rise-erpelding rise-erpelding merged commit dc5f820 into spectrum-two Jul 29, 2025
13 checks passed
@rise-erpelding rise-erpelding deleted the rise-erpelding/accordion-followups branch July 29, 2025 17:54
@github-actions github-actions bot mentioned this pull request Jul 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review S2 Spectrum 2 size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants