feat(expansion): add accordion expand/collapse all (#6929) #7461

henetiriki · 2017-10-02T00:42:09Z

Add expandAll()/collapseAll() to perform expand/collapse all on multiple expandable accordions.

amcdnl

Looks good overall, just a few nit.

amcdnl · 2017-10-02T14:02:20Z

src/lib/expansion/accordion.ts

+  /**
+   * Expands all enabled expansion panels in an accordion where multi is enabled
+   */
+  expandAll() {


Add return type void

amcdnl · 2017-10-02T14:02:24Z

src/lib/expansion/accordion.ts

+  /**
+   * Collapses all enabled expansion panels in an accordion where multi is enabled
+   */
+  collapseAll() {


Add return type void

amcdnl · 2017-10-02T14:02:29Z

src/lib/expansion/accordion.ts

+    this._expandCollapseAll(false);
+  }
+
+  _expandCollapseAll(expanded: boolean) {


Add return type void

jelbourn · 2017-10-02T14:45:52Z

src/demo-app/expansion/expansion-demo.ts


 @Component({
  moduleId: module.id,
  selector: 'expansion-demo',
  styleUrls: ['expansion-demo.css'],
  templateUrl: 'expansion-demo.html',
  encapsulation: ViewEncapsulation.None,
-  preserveWhitespaces: false,
+  preserveWhitespaces: true,


This needs to stay false

jelbourn · 2017-10-02T14:47:18Z

src/lib/expansion/accordion.ts

@@ -25,7 +26,10 @@ export class CdkAccordion {
  /** A readonly id value to use for unique selection coordination. */
  readonly id = `cdk-accordion-${nextId++}`;

-  /** Whether the accordion should allow multiple expanded accordion items simulateously. */
+  /** Stream that emits when expandAll or collapseAll is triggered. */
+  readonly expandCollapseAllSubject: Subject<boolean> = new Subject<boolean>();


Mark this with an underscore to denote that it is internal-only and add a JsDoc description (specifically that the boolean denotes expanded/collapsed)

jelbourn · 2017-10-02T14:48:26Z

src/lib/expansion/expansion-panel-header.ts

+    // When a panel is hosted in an accordion, subscribe to the expand/collapse subject
+    if (this.panel.accordion) {
+      this._expandCollapseAllSubscription =
+        this.panel.accordion.expandCollapseAllSubject.subscribe((expanded: boolean) => {


The type for expanded doesn't need to be given here because it is inferred from the observable.

jelbourn · 2017-10-02T14:49:47Z

src/lib/expansion/expansion-panel-header.ts

+
+    // When a panel is hosted in an accordion, subscribe to the expand/collapse subject
+    if (this.panel.accordion) {
+      this._expandCollapseAllSubscription =


Can you move the contents of this if block to a separate method? Something like _subscribeToAccordionActions

josephperrott · 2017-10-02T17:52:19Z

src/lib/expansion/expansion-panel-header.ts

@@ -101,6 +102,17 @@ export class MatExpansionPanelHeader implements OnDestroy {
    .subscribe(() => this._changeDetectorRef.markForCheck());

    _focusMonitor.monitor(_element.nativeElement, renderer, false);
+
+    // When a panel is hosted in an accordion, subscribe to the expand/collapse subject
+    if (this.panel.accordion) {


By placing the logic for setting panel expansion state in the MatExpansionPanelHeader it ties the logic specifically to the Material implementation rather than the CDK base.

I would suggestion adding this logic to the AccordionItem class.

There you can set the expanded state directly.

I need access to the disabled state of the hosting panel to prevent disabled panels from being expanded/collapsed, which is why I used _toggle(). Is there another way to get access to this property without too much refactoring?

Think I may have solved it with mixinDisabled - let me know if that is not a desired solution though. Cheers.

We actually just removed the mixinDisabled from AccordionItem as we are moving it to CDK. You can see the move in #7530

I supposed you could bake the expandAll/collapseAll into the AccordionItem, and then in MatExpansionPanel, extend the setter of expanded to respect disabled. This would allow the expandAll/collapseAll functionality to live in the CDK, but work as expected in MatAccordion. Admittedly I haven't thought this idea entirely through and would need to consider the ramifications a bit more before saying its definitely the best option or the option to go with.

Thanks, @josephperrott - I will wait for #7530 to be merged and then take it from there.

josephperrott · 2017-10-02T17:53:41Z

src/lib/expansion/expansion-panel-header.ts

+          // Only toggle if current state is not the intended state
+          if (!!this._isExpanded() !== expanded) {
+            this._toggle();
+          }


Coupled with the move to AccordionItem, I would suggest just setting the expanded state directly rather than checking if a toggle needs to occur.

this.panel.accordion.expandCollapseAllSubject.subscribe(expanded => this.expanded = expanded);

josephperrott · 2017-10-02T17:54:46Z

src/lib/expansion/accordion.ts

@@ -44,6 +48,26 @@ export class CdkAccordion {
   *     elevation.
   */
  @Input() displayMode: MatAccordionDisplayMode = 'default';
+
+  /**
+   * Expands all enabled expansion panels in an accordion where multi is enabled


JsDoc comments should be complete sentences so add a . here?

henetiriki · 2017-10-02T18:27:40Z

Thanks for the feedback @amcdnl, @jelbourn and @josephperrott - will get this sorted asap.

Unable to figure out if the Travis Sauce Labs failures are just platform problems or issues with this ticket.

Also, do I need to take any action on the Screenshot Tests?

josephperrott · 2017-10-02T19:27:39Z

@ouq77 We have been having some issues with our saucelab tests, I don't think its from you. For the screenshot tests, no need to worrry about them unless they are we are seeing failures in the expansion panels.

Once we get to a LGTM state on this PR, I can make the screenshot tests as good, however I want to wait as it will reset after every new push to the branch.

henetiriki · 2017-10-17T00:33:42Z

@josephperrott I attempted your suggestion of adding the functionality in the CDK, but couldn't find an elegant way of accessing the disabled property on the accordion item.

Since this is mainly a Material concern, leaving it in this layer is probably acceptable.

Let me know if this is not an ideal and I can rethink it.

Is there any chance that the common-behaviours will be moved to the CDK?

josephperrott · 2017-10-19T16:14:20Z

I spoke with @jelbourn and there does not seem to be an issue with moving some of the common-behaviors into CDK. disabled as an example would make sense to do so. We might need to move disabled and CanDisable into CDK which would open up all of the logic for expandAll/collapseAll to be in CDK.

henetiriki · 2017-10-24T17:50:52Z

Is this something I can help with @josephperrott? Should probably be done against a separate issue?

josephperrott · 2017-10-30T15:52:58Z

@ouq77 You definitely can move it to the CDK if you would like. It should be done in another pull ideally.

jelbourn · 2018-01-24T20:02:01Z

@josephperrott should this PR be revisited?

henetiriki · 2018-01-24T20:04:31Z

@josephperrott and @jelbourn - this PR is blocked until (and will be reworked after) #8201 has been dealt with. I've been keeping that one up to date hoping it will be revisited soon. Cheers.

josephperrott · 2018-01-25T16:34:02Z

PR is now unblocked with the merge of #8201

henetiriki · 2018-01-26T22:43:58Z

Knew I forgot something - updated now :)

josephperrott

Just the one comment, then it looks good to me. Though the change to naming will likely have some precipitating effects.

josephperrott · 2018-01-26T22:55:46Z

src/cdk/accordion/accordion.ts

@@ -28,4 +32,20 @@ export class CdkAccordion {
  get multi(): boolean { return this._multi; }
  set multi(multi: boolean) { this._multi = coerceBooleanProperty(multi); }
  private _multi: boolean = false;
+
+  /** Expands all enabled accordion items in an accordion where multi is enabled. */
+  expandAll(): void {


These should likely be openAll and closeAll to match the naming convention used in CdkAccordionItem.

josephperrott

LGTM

josephperrott · 2018-01-29T16:19:20Z

src/cdk/accordion/accordion-item.ts

@@ -125,4 +134,13 @@ export class CdkAccordionItem implements OnDestroy {
      this.expanded = true;
    }
  }
+
+  private _subscribeToExpandCollapseAllActions(): Subscription {


This method should likely be called subscribetoOpenCloseAllActions()

josephperrott · 2018-01-29T16:20:00Z

src/cdk/accordion/accordion.ts

+    this._expandCollapseAll(false);
+  }
+
+  private _expandCollapseAll(expanded: boolean): void {


This should be _openCloseAll()

Adds openAll()/closeAll() to perform expand/collapse all on multiple expandable accordions. Closes angular#6929

SchnWalter · 2018-01-30T15:01:17Z

src/cdk/accordion/accordion.ts

+  }
+
+  private _openCloseAll(expanded: boolean): void {
+    if (this.multi) {


There are some cases, even for [multi]="false", where you don't care which CdkAccordionItem is open, you just want to make sure that all are closed and it would be nice to also cover that use case, but I'm not sure how this could be done without confusing developers.

I guess you could do this.multi || !expanded? Will have to experiment.

That would be a solution, but we could have a "cleaner" solution, if I may use this word.

This would be a good solution for this particular case. But maybe we could find a more generic solution that could be used in other cases and not confuse people to much, openAll() won't work in single mode, but we could have open() that works for single mode, opening the first item or in multi mode.

If you look at something like the SelectionModel for tables, select and chips, the selection has a getMultipleValuesInSingleSelectionError, we could use something like that. And MatSelectionList.selectedOptions exposes the model for anyone to use. But probably a model would be an overkill for this situation, I'm not sure in how many places we could reuse it.

Also, MatDrawerContainer container has a similar behavior but it doesn't add the All suffix and it uses @ContentChildren to keep a list of children and then call open/close on each children, instead of emitting events, here's a snippet from it:

@ContentChildren(MatDrawer) _drawers: QueryList<MatDrawer>; /** Calls `open` of both start and end drawers */ open(): void { this._drawers.forEach(drawer => drawer.open()); } /** Calls `close` of both start and end drawers */ close(): void { this._drawers.forEach(drawer => drawer.close()); }

Using ContentChildren instead of emitting an event and rename to open/close in order to support "multi/single" mode, would be great.

P.S. Thanks for the work you did on this PR!

This maybe warrants a separate issue to investigate?

pankajsri03 · 2018-02-27T10:06:44Z

I created a demo using the example in the demo-app , but expand-all/collapse-all isn't working.
https://stackblitz.com/edit/angular-jmitne?file=app/expansion-overview-example.html.
This gives the error _co.accordion.openAll is not a function

henetiriki · 2018-02-27T19:30:44Z

@pankajsri03 - you are using cdk and material v5.2.1 in your example. This change will only be available in v6. (It is currently in beta)

zijianhuang · 2018-05-17T01:10:16Z

@pankajsri03 and @ouq77
I am using NG6.0.2 and NGMD6.0.2, and see the same "openAll is not a function".

"dependencies": { "@angular/animations": "^6.0.2", "@angular/cdk": "^6.0.2", "@angular/common": "^6.0.2", "@angular/compiler": "^6.0.2", "@angular/compiler-cli": "^6.0.2", "@angular/core": "^6.0.2", "@angular/flex-layout": "^6.0.0-beta.15", "@angular/forms": "^6.0.2", "@angular/material": "^6.0.2", "@angular/material-moment-adapter": "^6.0.2", "@angular/platform-browser": "^6.0.2", "@angular/platform-browser-dynamic": "^6.0.2", "@angular/platform-server": "^6.0.2", "@angular/router": "^6.0.2",

                    <mat-accordion #consultsAccordion multi="true">
                        <consult-item *ngFor="let c of consults" [model]="c" [filter]="filter"></consult-item>
                    </mat-accordion>

    @ViewChild('consultsAccordion') consultPanels: MatAccordion;
...
    this.consultPanels.openAll();

zijianhuang · 2018-06-08T04:51:07Z

As of NGMD 6.2.1, the problem persist: "openAll is not a function". Do I miss something? or someone in the NGMD team had forgotten to merge?

henetiriki · 2018-06-08T23:42:33Z

@zijianhuang I assume your consult-item component contains the mat-expansion-panel? I may be wrong, but for any of the expansion panel logic to work as expected, they need to be hosted directly inside the mat-accordion.

zijianhuang · 2018-06-10T07:16:34Z

@ouq77, yes, the root element of consult-item is
<mat-expansion-panel (opened)="handleOpened()" (closed)="handleClosed()">

So, after NG rendering, mat-expansion-panel is hosted directly inside the mat-accordion.

zijianhuang · 2018-07-03T01:24:37Z

In the other parts of the program, I do have mat-expansion-panel directly inside the mat-accordion, however, openalll is still not function.

After a few NG and MD releases, this is still not fixed. Now I just go with my own workaround or solution: publish an event, and all expansion panels subscribe it; when the parent component fire the event, all subscribers just open the expansion panels.

angular-automatic-lock-bot · 2019-09-09T04:03:33Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

henetiriki requested review from amcdnl, jelbourn and josephperrott as code owners October 2, 2017 00:42

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 2, 2017

henetiriki force-pushed the feature/6929-accordion-expand-collapse-all branch from 835b6fb to 1a41208 Compare October 2, 2017 05:52

amcdnl reviewed Oct 2, 2017

View reviewed changes

jelbourn reviewed Oct 2, 2017

View reviewed changes

josephperrott self-assigned this Oct 2, 2017

josephperrott reviewed Oct 2, 2017

View reviewed changes

henetiriki force-pushed the feature/6929-accordion-expand-collapse-all branch 2 times, most recently from 9c95e7b to 1bd3b61 Compare October 6, 2017 04:00

henetiriki force-pushed the feature/6929-accordion-expand-collapse-all branch 6 times, most recently from 18f0593 to e4dca02 Compare October 16, 2017 22:55

henetiriki mentioned this pull request Oct 31, 2017

[accordion]: add disabled property to CDK accordion-item #8141

Closed

josephperrott added the pr: needs rebase label Nov 14, 2017

josephperrott added blocked This issue is blocked by some external factor, such as a prerequisite PR and removed blocked This issue is blocked by some external factor, such as a prerequisite PR labels Jan 24, 2018

henetiriki force-pushed the feature/6929-accordion-expand-collapse-all branch 3 times, most recently from 23cc812 to 1b2aaa8 Compare January 26, 2018 22:19

henetiriki force-pushed the feature/6929-accordion-expand-collapse-all branch from 1b2aaa8 to 0bff53f Compare January 26, 2018 22:55

josephperrott reviewed Jan 26, 2018

View reviewed changes

henetiriki force-pushed the feature/6929-accordion-expand-collapse-all branch 3 times, most recently from 12ab88a to 12e74a9 Compare January 26, 2018 23:36

josephperrott requested changes Jan 29, 2018

View reviewed changes

feat(expansion): add accordion expand/collapse all (angular#6929)

7c2e39e

Adds openAll()/closeAll() to perform expand/collapse all on multiple expandable accordions. Closes angular#6929

henetiriki force-pushed the feature/6929-accordion-expand-collapse-all branch from 12e74a9 to 7c2e39e Compare January 29, 2018 17:58

josephperrott approved these changes Jan 29, 2018

View reviewed changes

josephperrott added pr: lgtm action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release and removed pr: needs rebase labels Jan 29, 2018

SchnWalter reviewed Jan 30, 2018

View reviewed changes

mmalerba merged commit 3aceb73 into angular:master Feb 9, 2018

henetiriki deleted the feature/6929-accordion-expand-collapse-all branch February 9, 2018 22:30

zijianhuang mentioned this pull request Jul 3, 2018

Bug: MatAccordion .openAll() is not a function #12025

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019

feat(expansion): add accordion expand/collapse all (#6929) #7461

feat(expansion): add accordion expand/collapse all (#6929) #7461

Uh oh!

Conversation

henetiriki commented Oct 2, 2017

Uh oh!

amcdnl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henetiriki Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henetiriki Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henetiriki commented Oct 2, 2017

Uh oh!

josephperrott commented Oct 2, 2017

Uh oh!

henetiriki commented Oct 17, 2017

Uh oh!

josephperrott commented Oct 19, 2017

Uh oh!

henetiriki commented Oct 24, 2017

Uh oh!

josephperrott commented Oct 30, 2017

Uh oh!

jelbourn commented Jan 24, 2018

Uh oh!

henetiriki commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephperrott commented Jan 25, 2018

Uh oh!

henetiriki commented Jan 26, 2018

Uh oh!

josephperrott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephperrott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henetiriki Oct 2, 2017 •

edited

Loading

henetiriki Oct 9, 2017 •

edited

Loading

henetiriki commented Jan 24, 2018 •

edited

Loading

SchnWalter Jan 30, 2018 •

edited

Loading

SchnWalter Jan 31, 2018 •

edited

Loading

zijianhuang commented May 17, 2018 •

edited

Loading