`Accordion` - expand/collapse enhancements #2147

alex-ju · 2024-06-07T11:52:07Z

📌 Summary

In this PR we propose a set of enhancements to our Accordion component to accommodate the following use cases without introducing any breaking changes:

Allow consumers to implement an "expand all"/"collapse all" functionality (request reference)

we considered embedding the button inside the component but decided that would be too restrictive for our consumers
the flexible implementation also allows consumers to control the state (open/close) of each accordion item from outside the component

Example of usage

<button type="button" {{on "click" this.toggleState}}>
{{if (eq this.state "open") "Collapse all" "Expand all"}}
</button>

<Hds::Accordion @forceState={{this.state}} as |A|>
{{! accordion items }}
</Hds::Accordion>

Expose a function to close an accordion item from within its content (request reference)

Example of usage

<Hds::Accordion::Item>
  <:toggle>Item one</:toggle>
  <:content as |c|>
     <button type="button" {{on "click" c.close}}>Close</button>
  </:content>
</Hds::Accordion::Item>

🛠️ Detailed description

Updated the DisclosurePrimitive to allow external control of the isOpen state. We do this by introducing two private properties (_isOpen, representing the internal state and _isControlled set to true every time the @isOpen argument is changed externally). Tried to follow the concepts previously explored.
Updated the Accordion::Item and Accordion adding a @forceState argument (open/close) to control the state from outside the component after the initial render.
Updated the Accordion::Item component to forward the close action from DisclosurePrimitive.

🔗 External links

Jira ticket: HDS-3364

👀 Component checklist

Make the type adjustments in the relevant types.ts files
Extend test suites to cover the new functionality
Test/validate the solution with consumers
Percy was checked for any visual regression
A changelog entry was added via Changesets if needed (see templates here)

💬 Please consider using conventional comments when reviewing this PR.

vercel · 2024-06-07T11:52:09Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
hds-showcase	✅ Ready (Inspect)	Visit Preview	Jun 25, 2024 11:51am
hds-website	✅ Ready (Inspect)	Visit Preview	Jun 25, 2024 11:51am

zamoore

Given the requirements, this approach makes sense to me.

zamoore · 2024-06-10T13:31:05Z

packages/components/src/components/hds/accordion/item/index.ts

+    content: [
+      {
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        close: (...args: any[]) => void;


Could this be unknown instead of any?

Oh interesting (unknown vs. any). Was looking at this: https://devblogs.microsoft.com/typescript/announcing-typescript-3-0-rc-2/#the-unknown-type

Seems like a better choice if it works for us.

unknown might be a better fit although, for consistency with other functions, I would be tempted to leave it as any and review them holistically.

zamoore · 2024-06-10T13:32:35Z

packages/components/src/components/hds/disclosure-primitive/index.ts

+  }
+
+  @action
+  onStateChange() {


It looks like you're passing @isOpen from the template as well.

Yes, @isOpen can be accessed as an argument, but it's not required. The reason I pass it in from the template is for the ember render modifier to track it

packages/components/src/components/hds/disclosure-primitive/index.ts

didoo

@alex-ju added a comment that may reveal to be a blocker, have a look

packages/components/src/components/hds/accordion/index.ts

packages/components/src/components/hds/disclosure-primitive/index.ts

didoo

Left a few comments and considerations.

I haven't completed the review, because I want to discuss with you the controlled/uncontrolled states (still doesn't work for me, but maybe it's how I am thinking of the problem). Let's sync later in the day

didoo · 2024-06-20T06:11:03Z

packages/components/src/components/hds/accordion/index.hbs

@@ -4,5 +4,5 @@
 }}

 <div class="hds-accordion" ...attributes>
-  {{yield (hash Item=(component "hds/accordion/item"))}}
+  {{yield (hash Item=(component "hds/accordion/item" forceState=@forceState))}}


playing advocate's devil: should this new argument called something like forceIsOpen (or forceOpen) and be a boolean? 🤔

another consideration: I was imagining that the forced state would be at the item level, not at the parent accordion level (the isOpen is at item level). why is it necessary at this level? one can achieve the same "collapse all/expand all" by passing the @forceState argument to all the items, plus (in that case) it's possible to control a single item independently from the others (while with the current implementation it's not possible)

my understanding is that our consumers want a way to manage the states of all accordion items (not individual items), hence the approach of setting it at the root and cascading it to items.

updated with the suggested adjustments, adding onClickToggle to DisclosurePrimitive and forwarding it to Accordion::Item

what about having the forceState argument as boolean (calling it something like forceIsOpen or forceOpen)? would it make sense? do we expect to have more than one type of forced state in the future?

packages/components/src/components/hds/accordion/item/index.hbs

Co-Authored-By: Cristiano Rastelli <686239+didoo@users.noreply.github.com>

to showcase `onClickToggle` Co-Authored-By: Cristiano Rastelli <686239+didoo@users.noreply.github.com>

Co-Authored-By: Cristiano Rastelli <686239+didoo@users.noreply.github.com>

didoo

Left a few more comments

didoo · 2024-06-25T13:30:15Z

packages/components/src/components/hds/accordion/index.hbs

@@ -4,5 +4,5 @@
 }}

 <div class="hds-accordion" ...attributes>
-  {{yield (hash Item=(component "hds/accordion/item"))}}
+  {{yield (hash Item=(component "hds/accordion/item" forceState=@forceState))}}


what about having the forceState argument as boolean (calling it something like forceIsOpen or forceOpen)? would it make sense? do we expect to have more than one type of forced state in the future?

didoo · 2024-06-25T14:38:48Z

showcase/tests/integration/components/hds/accordion/index-test.js

+      `
+    );
+    // first item open at rendering
+    assert.dom('.hds-accordion-item__content').exists({ count: 1 });


being a bit over-zealous here: should we be more specific to make sure it's the first item being open?

didoo · 2024-06-25T14:41:48Z

showcase/tests/integration/components/hds/accordion/index-test.js

+        <:content as |c|>
+          <button type="button" {{on "click" c.close}}>Close</button>
+        </:content>


Suggested change

<:content as |c|>

<button type="button" {{on "click" c.close}}>Close</button>

</:content>

<:content as |c|>Content one</:content>

didoo · 2024-06-25T14:43:25Z

showcase/tests/integration/components/hds/accordion/index-test.js

+    assert.dom('.hds-accordion-item__content').doesNotExist();
+    // toggle to open
+    await click('.hds-accordion-item__button');
+    assert.dom('.hds-accordion-item__content').exists();


I think this is not really testing the onClickToggle callback; probably we should have an "external state" that gets changed on onClickToggle and check that external state value?

To make sure I wasn't missing something, I've commented out the content of the this.set('onClickToggle', () => {}); function, and the tests are still passing.

didoo · 2024-06-25T20:39:31Z

packages/components/src/components/hds/disclosure-primitive/index.ts

+      // if the state is controlled from outside, the argument overrides the internal state
+      return this.args.isOpen ?? this._isOpen;
+    } else {
+      // if the state is changes internally, the internal state overrides the argument


Suggested change

// if the state is changes internally, the internal state overrides the argument

// if the state changes internally, the internal state overrides the argument

didoo · 2024-06-25T20:42:24Z

packages/components/src/components/hds/disclosure-primitive/index.ts

+      return this.args.isOpen ?? this._isOpen;
+    } else {
+      // if the state is changes internally, the internal state overrides the argument
+      return this._isOpen !== undefined


in which case the value of _isOpen would be undefined? (I did a check, and to me seems it's not possible)

didoo · 2024-06-25T20:43:37Z

packages/components/src/components/hds/disclosure-primitive/index.ts

@@ -33,11 +35,44 @@ export interface HdsDisclosurePrimitiveSignature {
 }

 export default class HdsDisclosurePrimitiveComponent extends Component<HdsDisclosurePrimitiveSignature> {
-  @tracked isOpen = this.args.isOpen ?? false;
+  @tracked _isOpen = false;
+  @tracked _isControlled = this.args.isOpen != undefined;


Suggested change

@tracked _isControlled = this.args.isOpen != undefined;

@tracked _isControlled = this.args.isOpen !== undefined;

didoo · 2024-06-25T21:00:25Z

packages/components/src/components/hds/disclosure-primitive/index.ts

+
+  @action
+  onStateChange() {
+    if (this.args.isOpen !== undefined) {


if I understand correctly, we don't use this._isControlled here because this function is invoked when the value of @isOpen changes, which triggers the did-update which is run without re-rendering the component, so line 39 is not re-evaluated

if that is the case, what do you think of adding a comment to explain this "detail"? 😀 (it took me a while to get it)

if not, then maybe we can use this._isControlled as condition? would make sense (in which case TS complains, but we can cast the assignment via Boolean(this.args.isOpen) like we do at line 49

didoo · 2024-06-25T21:01:50Z

showcase/app/templates/components/accordion.hbs

+      </Hds::Accordion>
+    </SG.Item>
+  </Shw::Grid>
+
  <Shw::Text::H3>Edge cases</Shw::Text::H3>


Suggested change

<Shw::Text::H3>Edge cases</Shw::Text::H3>

<Shw::Divider @level={{2}} />

<Shw::Text::H3>Edge cases</Shw::Text::H3>

emailnitram · 2024-06-25T22:58:18Z

I tested this PR via the deployed showcase link https://hds-showcase-git-alex-ju-accordion-expand-all-hashicorp.vercel.app/components/accordion#externally-controlled and https://hds-showcase-git-alex-ju-accordion-expand-all-hashicorp.vercel.app/utilities/disclosure-primitive and the functionality and API looks good 👍

hashibot-hds added packages/components showcase labels Jun 7, 2024

alex-ju force-pushed the alex-ju/accordion-expand-all branch from ba65850 to 5b56b0c Compare June 7, 2024 12:13

vercel bot deployed to Preview – hds-showcase June 7, 2024 12:16 View deployment

vercel bot deployed to Preview – hds-website June 7, 2024 12:17 View deployment

alex-ju requested a review from a team June 7, 2024 14:16

zamoore reviewed Jun 10, 2024

View reviewed changes

didoo reviewed Jun 11, 2024

View reviewed changes

packages/components/src/components/hds/accordion/index.ts Outdated Show resolved Hide resolved

packages/components/src/components/hds/disclosure-primitive/index.ts Show resolved Hide resolved

alex-ju changed the title ~~Accordion enhancements~~ Accordion - expand/collapse enhancements Jun 12, 2024

alex-ju force-pushed the alex-ju/accordion-expand-all branch from 5b56b0c to 50d2ddd Compare June 17, 2024 17:11

vercel bot deployed to Preview – hds-showcase June 17, 2024 17:14 View deployment

vercel bot deployed to Preview – hds-website June 17, 2024 17:15 View deployment

alex-ju added 4 commits June 19, 2024 16:30

Update DisclosurePrimitive to allow external control

1eca553

Update Accordion to allow external control via @forceState

6cc1757

Expose close function to Accordion::Item

211a212

Move type in a separate file

c38a3b6

alex-ju force-pushed the alex-ju/accordion-expand-all branch from 50d2ddd to c38a3b6 Compare June 19, 2024 16:18

vercel bot deployed to Preview – hds-showcase June 19, 2024 16:20 View deployment

vercel bot deployed to Preview – hds-website June 19, 2024 16:22 View deployment

alex-ju marked this pull request as ready for review June 19, 2024 16:32

alex-ju requested review from a team and emailnitram June 19, 2024 16:32

didoo reviewed Jun 20, 2024

View reviewed changes

alex-ju and others added 4 commits June 25, 2024 12:45

Add onClickToggle callback to DisclosurePrimitive

5e0fc39

Co-Authored-By: Cristiano Rastelli <686239+didoo@users.noreply.github.com>

Update external controlled example for DisclosurePrimitive

d3cb679

to showcase `onClickToggle` Co-Authored-By: Cristiano Rastelli <686239+didoo@users.noreply.github.com>

Forward onClickToggle to Accordion::Item

760ebf3

Co-Authored-By: Cristiano Rastelli <686239+didoo@users.noreply.github.com>

Update forced state examples to showcase onClickToggle

16f6d80

Co-Authored-By: Cristiano Rastelli <686239+didoo@users.noreply.github.com>

vercel bot deployed to Preview – hds-showcase June 25, 2024 11:49 View deployment

vercel bot deployed to Preview – hds-website June 25, 2024 11:51 View deployment

alex-ju requested review from jgwhite and didoo June 25, 2024 12:30

didoo reviewed Jun 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Accordion` - expand/collapse enhancements #2147

`Accordion` - expand/collapse enhancements #2147

alex-ju commented Jun 7, 2024 •

edited

Loading

vercel bot commented Jun 7, 2024 •

edited

Loading

zamoore left a comment

zamoore Jun 10, 2024

KristinLBradley Jun 10, 2024

alex-ju Jun 17, 2024

zamoore Jun 10, 2024

alex-ju Jun 17, 2024

didoo left a comment •

edited

Loading

didoo left a comment

didoo Jun 20, 2024

didoo Jun 20, 2024

alex-ju Jun 20, 2024

alex-ju Jun 25, 2024

didoo Jun 25, 2024

didoo left a comment

didoo Jun 25, 2024

didoo Jun 25, 2024

didoo Jun 25, 2024

didoo Jun 25, 2024

didoo Jun 25, 2024

didoo Jun 25, 2024

didoo Jun 25, 2024

didoo Jun 25, 2024

didoo Jun 25, 2024

didoo Jun 25, 2024

emailnitram commented Jun 25, 2024

	// if the state is changes internally, the internal state overrides the argument
	// if the state changes internally, the internal state overrides the argument

	@tracked _isControlled = this.args.isOpen != undefined;
	@tracked _isControlled = this.args.isOpen !== undefined;

Accordion - expand/collapse enhancements #2147

Are you sure you want to change the base?

Accordion - expand/collapse enhancements #2147

Conversation

alex-ju commented Jun 7, 2024 • edited Loading

📌 Summary

🛠️ Detailed description

🔗 External links

👀 Component checklist

vercel bot commented Jun 7, 2024 • edited Loading

zamoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didoo left a comment • edited Loading

Choose a reason for hiding this comment

didoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emailnitram commented Jun 25, 2024

`Accordion` - expand/collapse enhancements #2147

`Accordion` - expand/collapse enhancements #2147

alex-ju commented Jun 7, 2024 •

edited

Loading

vercel bot commented Jun 7, 2024 •

edited

Loading

didoo left a comment •

edited

Loading