fix: flush state for accordion component

[riddhybansal]

Closes #15849

Accordion with flush alignment does not meet design spec

Changed

• flush styles
• fix for skeleton state for accordion

Testing / Reviewing

Go to Playground for accordion component, enable flush property, border should come in shorter to match flush spec

[netlify]

✅ Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	7240091
🔍 Latest deploy log	https://app.netlify.com/sites/v11-carbon-react/deploys/664cbad55653010008a4e4c2
😎 Deploy Preview	https://deploy-preview-15993--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.

[alina-jacob]

Hi @riddhybansal great work! 💯
Just one tiny observation 👀

This is the intended specs for the Hover state of Flush Accordion

The deploy preview link has the horizontal divider stand out in hover state, is there a way to blend it the way it works for the regular accordion?
Also in the preview it looks like the chevron goes missing on hover state, not sure why, but could you take a look!
The chevron up to chevron down interaction is also missing, maybe that's solved if the hover state issue is fixed.

[tw15egan]

@alina-jacob it seems like the divider is still there on hover when not isFlush, but it is less noticeable since the divider and hover area have the same surface area.

Should this be removed in the normal, non isFlush variant as well?

@riddhybansal it seems like we need to add a transition property to the pseudo element, as it is currently instantly appearing / disappearing and causing a flicker on hover

[alina-jacob]

@alina-jacob it seems like the divider is still there on hover when not isFlush, but it is less noticeable since the divider and hover area have the same surface area.

Should this be removed in the normal, non isFlush variant as well?

@riddhybansal it seems like we need to add a transition property to the pseudo element, as it is currently instantly appearing / disappearing and causing a flicker on hover

yes ✅ @tw15egan, @riddhybansal
the horizontal divider should be removed/hidden during hover states in flush and default modes for the Accordion.

here's a Figma ref file that @laurenmrice has created and here's Accordion's style documentation from the Carbon website indicating the same.

[riddhybansal]

@alina-jacob the issue regarding the hover states are fixed and the issue of divider for both the flush and non flush states are addressed in the same PR

[alisonjoseph]

Removing the border on hover is causing a layout shift. You may have to update the color of the border to match the background?

Screen.Recording.2024-03-26.at.8.24.59.AM.mov

[alisonjoseph]

Taking a closer look I'm still seeing a weird flash/flickr on hover, however I want to confirm with design that the way its hanging outside of the content area is correct. Or should the text align between the two variants with the divider lines moved in, vs. the hover/focus area pushed out.

@alina-jacob or @carbon-design-system/design can you weigh in here. This is a screen shot with the padding we add for all our storybook stories removed. You can see that the isFlush variant hangs outside the normal content area.

Compared with how we handle something similar with ContainedList

[alina-jacob]

Hi @alisonjoseph,
Based on the deploy preview, the flush execution seems okay.
Is it an issue if the hover/active/focus state hangs out of the content area from a dev perspective?

• Riddhi has pushed the text and icons to the left and right to meet the width of the divider.
• The contained list screenshot that you have shared, that has the line pushed in to meet the text and the icons.
  (is one better than the other from a dev perspective, because design wise, the current way also works)

[alisonjoseph]

My opinion is that we wouldn't want any focus/hover to hang outside of the content area. @tw15egan @laurenmrice thoughts?

If we got rid of the hang it would either look like this, which is odd. Or it would have to push in, which might not be the intended alignment?

Or maybe its totally fine to have the focus/hover outside the content area.

[laurenmrice]

For context, this is the type of area in a UI where our users are using the flush accordion to achieve better vertical text alignment. The focus and hover states would hit the edges of the left panel container, and those states would extend outside of the divider borders between the rows. People should not mix the flush alignment with the default alignment within one accordion group.

And yes, to Alison's point above, we do not want to remove the hang for the hover and focus states for this because it would create visual tension between the edges of the state containers and the text.

[alisonjoseph]

@laurenmrice thank you so much for the example! That clears things up for me.

@riddhybansal If you see the in-context example above from Lauren you'll see how it should display in a container. So you'll want to adjust the styles so that the appearance of the border shifts inward by 1rem on either side instead of the entire accordion shifting outward by 1rem.

Currently the border is set on .cds--accordion__item, for this you'll have to remove that border and use psuedo-elements instead. You'll need to do something similar for the hover on .cds--accordion__heading. Something similar to below. Let me know if you get stuck/need to pair up.

[alisonjoseph]

@riddhybansal whats the status on this one? Let me know if you need to pair up.

[alisonjoseph]

@riddhybansal I pushed some updates to this PR, we can talk through in a webex if needed.

I added a test story so we can view the flush variant side by side, I also updated/fixed skeleton states. (will need to remove test story before merging)

[aagonzales]

Oh wow lots of comments in this PR but it looks like everything has been addressed. Final demo is working as expected.

stale

[netlify]

✅ Deploy Preview for carbon-elements ready!

Name	Link
🔨 Latest commit	7240091
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-elements/deploys/664cbad5bbc0ae00083065ba
😎 Deploy Preview	https://deploy-preview-15993--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.

[carbon-automation]

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