feat(expansion): allow expansion indicator positioning. #8199

josephperrott · 2017-11-02T18:17:50Z

Allows expansion indicator to be positioned at start or end of expansion panel header.

crisbeto

In addition to the couple of comments below, it doesn't look like this handles RTL.

crisbeto · 2017-11-02T18:31:26Z

src/lib/expansion/accordion.ts


 /** Workaround for https://github.com/angular/angular/issues/17849 */
 export const _CdkAccordion = CdkAccordion;

 /** MatAccordion's display modes. */
 export type MatAccordionDisplayMode = 'default' | 'flat';

+
+ /** MatAccordion's toggle positions. */
+ export type MatAccordionTogglePosition = 'start' | 'end';


Everywhere else we use before and after for these kinds of inputs.

crisbeto · 2017-11-02T18:32:30Z

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

@@ -3,5 +3,7 @@
  <ng-content select="mat-panel-description"></ng-content>
  <ng-content></ng-content>
 </span>
-<span [@indicatorRotate]="_getExpandedState()" *ngIf="_showToggle()"
-      class="mat-expansion-indicator"></span>
+<div class="mat-expansion-indicator-container" [style.order]="_indicatorOrder()">


It might be more appropriate to toggle a class that sets the order. It also makes it easier to handle RTL.

josephperrott · 2017-11-02T18:47:51Z

Comments addressed, RTL is handled by CSS order of flex items in the flex container.

crisbeto · 2017-11-02T19:03:42Z

src/lib/expansion/accordion.ts

+
+  /** The positioning of the expansion indicator. */
+  @Input() togglePosition: MatAccordionTogglePosition = 'after';
+


Optional: there a bit too many newlines above and below this input.

crisbeto · 2017-11-02T19:05:18Z

src/lib/expansion/expansion.spec.ts

+      expect(arrow.style.transform).toBe('rotate(225deg)', 'Expected 225 degree rotation.');
+    }));
+
+  it('should update the indicator position when the position is set programmatically',


Also add a test that verifies that the position still works when it's set on the accordion?

Also added a test for displayMode since it looks like that was missing in the tests as well.

crisbeto

LGTM, apply the merge ready label when the lint warning is sorted out.

jelbourn · 2017-11-03T16:16:42Z

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

    this._parentChangeSubscription = merge(
      panel.opened,
      panel.closed,
-      panel._inputChanges.pipe(filter(changes => !!(changes.hideToggle || changes.disabled)))
+      merge(...changeSubjects).pipe(
+        filter(changes => !!(changes.hideToggle || changes.disabled || changes.togglePosition)))


You don't need !! in conditionals, only when the value is returned or stored for later

It looks like filter requires the value returned to be boolean, and without the coercion the returned type is SimpleChanges

jelbourn · 2017-11-03T16:17:48Z

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

    @Host() public panel: MatExpansionPanel,
    private _element: ElementRef,
    private _focusMonitor: FocusMonitor,
    private _changeDetectorRef: ChangeDetectorRef) {

+    let changeSubjects = [panel._inputChanges];


You don't care here that these are Subjects, just that they're observables. I'd call this changeStreams

jelbourn · 2017-11-03T16:19:41Z

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

    return !this.panel.hideToggle && !this.panel.disabled;
  }

+  _placeIndicatorBefore(): boolean {


Does this function serve any value?

This method determines the ordering of the content in the header.

jelbourn · 2017-11-03T16:20:02Z

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

@@ -132,10 +141,14 @@ export class MatExpansionPanelHeader implements OnDestroy {
  }

  /** Gets whether the expand indicator should be shown. */
-  _showToggle(): boolean {
+  _showIndicator(): boolean {


_isToggleVisible?

jelbourn · 2017-11-03T16:20:52Z

src/lib/expansion/expansion-panel.ts

+  /** The positioning of the expansion indicator. */
+  @Input()
+  get togglePosition(): MatAccordionTogglePosition {
+    return this.accordion ? this.accordion.togglePosition : this._togglePosition;


Shouldn't the more specific position trump the parent?

Currently the hideToggle value goes based on parent's value even if the child is defined so I went with the same. There is a strong argument that they both should be changed.

jelbourn

LGTM

josephperrott · 2017-11-20T17:40:44Z

Caretaker:

This change fixes the spacing of the expansion panel header to be even regardless of content of the title. This difference can be seen in the screenshot tests.

mmalerba · 2018-04-18T16:12:16Z

@josephperrott needs rebase, re-apply merge ready when done

ngbot · 2018-05-03T20:38:54Z

Hi @josephperrott! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

This reverts commit 51d859f.

mmalerba · 2018-05-22T22:36:00Z

@josephperrott This had to be reverted due to unexpected screenshot diffs in g3

johnchristopherjones · 2018-08-08T15:26:40Z

Any plans to complete this feature?

angular-automatic-lock-bot · 2019-09-09T09:03:05Z

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.}

josephperrott requested a review from jelbourn as a code owner November 2, 2017 18:17

josephperrott mentioned this pull request Nov 2, 2017

feat(expansion): Allow expansion indicator positioning #6917

Closed

josephperrott added the pr: needs review label Nov 2, 2017

josephperrott requested review from crisbeto and andrewseguin November 2, 2017 18:18

crisbeto reviewed Nov 2, 2017

View reviewed changes

josephperrott force-pushed the panel-indicator branch from c5dd84f to 6c79470 Compare November 2, 2017 18:46

josephperrott force-pushed the panel-indicator branch from 6c79470 to 82e8189 Compare November 2, 2017 18:52

crisbeto reviewed Nov 2, 2017

View reviewed changes

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

josephperrott force-pushed the panel-indicator branch 2 times, most recently from 58047c4 to 7411347 Compare November 3, 2017 15:05

crisbeto approved these changes Nov 3, 2017

View reviewed changes

crisbeto added pr: lgtm and removed pr: needs review labels Nov 3, 2017

jelbourn reviewed Nov 3, 2017

View reviewed changes

josephperrott force-pushed the panel-indicator branch from 7411347 to f201810 Compare November 6, 2017 19:20

jelbourn approved these changes Nov 8, 2017

View reviewed changes

jelbourn added the action: merge The PR is ready for merge by the caretaker label Nov 8, 2017

josephperrott force-pushed the panel-indicator branch 3 times, most recently from 1fdaca5 to 97e3989 Compare November 20, 2017 16:56

josephperrott added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Nov 20, 2017

josephperrott force-pushed the panel-indicator branch 2 times, most recently from 7b0e73e to 60d5455 Compare December 6, 2017 21:05

josephperrott added the target: minor This PR is targeted for the next minor release label Dec 6, 2017

josephperrott force-pushed the panel-indicator branch 2 times, most recently from 2431ffc to 6a3f7ca Compare January 8, 2018 18:11

josephperrott force-pushed the panel-indicator branch 2 times, most recently from 0788d32 to e8c77c1 Compare January 12, 2018 16:02

josephperrott force-pushed the panel-indicator branch from e8c77c1 to d300373 Compare January 25, 2018 23:26

josephperrott force-pushed the panel-indicator branch from d300373 to fb5565c Compare February 13, 2018 15:56

josephperrott force-pushed the panel-indicator branch 5 times, most recently from 46e0bd3 to 99e2701 Compare April 4, 2018 16:19

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Apr 18, 2018

josephperrott force-pushed the panel-indicator branch 6 times, most recently from b320840 to 1c3ff54 Compare May 3, 2018 00:46

Allow expansion indicator positioning.

7ba0aff

josephperrott force-pushed the panel-indicator branch from 1c3ff54 to 7ba0aff Compare May 3, 2018 21:12

josephperrott added the action: merge The PR is ready for merge by the caretaker label May 3, 2018

josephperrott removed the request for review from andrewseguin May 8, 2018 23:37

mmalerba merged commit 51d859f into angular:master May 22, 2018

mmalerba added a commit that referenced this pull request May 22, 2018

Revert "feat(expansion): allow expansion indicator positioning. (#8199)"

fbd8084

This reverts commit 51d859f.

mmalerba mentioned this pull request May 22, 2018

Revert "feat(expansion): allow expansion indicator positioning." #11466

Merged

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

josephperrott deleted the panel-indicator branch March 20, 2020 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(expansion): allow expansion indicator positioning. #8199

feat(expansion): allow expansion indicator positioning. #8199

josephperrott commented Nov 2, 2017

crisbeto left a comment

crisbeto Nov 2, 2017

crisbeto Nov 2, 2017

josephperrott Nov 2, 2017

josephperrott commented Nov 2, 2017

crisbeto Nov 2, 2017

crisbeto Nov 2, 2017

josephperrott Nov 3, 2017

crisbeto left a comment

jelbourn Nov 3, 2017

josephperrott Nov 6, 2017

jelbourn Nov 3, 2017

jelbourn Nov 3, 2017

josephperrott Nov 6, 2017

jelbourn Nov 3, 2017

jelbourn Nov 3, 2017

josephperrott Nov 6, 2017

jelbourn left a comment

josephperrott commented Nov 20, 2017

mmalerba commented Apr 18, 2018

ngbot bot commented May 3, 2018

mmalerba commented May 22, 2018

johnchristopherjones commented Aug 8, 2018

angular-automatic-lock-bot bot commented Sep 9, 2019


		/** The positioning of the expansion indicator. */
		@Input() togglePosition: MatAccordionTogglePosition = 'after';

feat(expansion): allow expansion indicator positioning. #8199

feat(expansion): allow expansion indicator positioning. #8199

Conversation

josephperrott commented Nov 2, 2017

crisbeto 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

josephperrott commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto 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

jelbourn left a comment

Choose a reason for hiding this comment

josephperrott commented Nov 20, 2017

mmalerba commented Apr 18, 2018

ngbot bot commented May 3, 2018

mmalerba commented May 22, 2018

johnchristopherjones commented Aug 8, 2018

angular-automatic-lock-bot bot commented Sep 9, 2019