feat(sort): add ability to manage and display sorting #5307

andrewseguin · 2017-06-22T19:08:31Z

Introduces the MdSort which contains MdSortable elements. The MdSort manages the active sort and direction while each MdSortable element changes and displays the sorting for its field.

MdSortHeader is the first MdSortable to be introduced, to be easily used within CdkTable.

jelbourn

Got some strict null errors

jelbourn · 2017-06-23T03:09:39Z

src/lib/sort/sort-header.ts

+
+  /** Overrides the reverse order value of the containing MdSort for this MdSortable. */
+  @Input()
+  get reverseOrder() { return this._reverseOrder; }


I don't follow what this does

For setting the sort order as

descending -> ascending -> [none] -> descending -> ...

as opposed to

ascending -> descending' -> [none] -> descending -> ...

Isn't that what start already does?

Not quite - start determines where in the cycle the user is placed when first clicked. reverseOrder will reverse the cycle.

Sounds like I misunderstood your intentions on start, which would reverse the order.

We could remove one or the other and we'd lose a minor and probably unnecessary cycle. I prefer keeping reverseOrder over start though

Yeah, my intent was that:
Default: asc -> desc -> none
Start = desc: desc -> asc -> none

I think having both of these is more confusing than helpful. The problem with "reverse" is that you have to know what the default is. Saying start or startWith, in my mind, better captures the developer's intent

SGTM - removed the reverse. Agreed that just start is enough

jelbourn · 2017-06-23T03:13:17Z

src/lib/sort/sort-header.html

@@ -0,0 +1,20 @@
+<div class="mat-sort-header-container"
+     [class.mat-sort-header-position-before]="arrowPosition == 'before'">
+  <button class="mat-sort-header-button"


Add type="button"

jelbourn · 2017-06-23T03:16:03Z

src/lib/sort/sort-header.ts

+  host: {
+    '(click)': '_sort.sort(this)',
+    '[class.mat-sort-header-sorted]': '_isSorted()',
+  }


Add ViewEncapsulation.None and ChangeDetectionStrategy.OnPush

jelbourn · 2017-06-23T03:26:06Z

src/lib/sort/sort-header.scss

+}
+
+.mat-sort-header-stem {
+  background: black;


Color should be part of the theme (or maybe just currentColor?)

jelbourn · 2017-06-23T03:29:41Z

src/lib/sort/sort-header.ts

+      this.id = this._cdkColumnDef.name;
+    }
+
+    this._sort.register(this);


Should be able to do this in the constructor

Cannot do this since the MdSort creates a mapping of id to MdSortable and this is the earlier lifecycle hook where we have the id available

jelbourn · 2017-06-23T03:30:35Z

src/lib/sort/sort-header.ts

+
+  constructor(@Optional() public _sort: MdSort,
+              public _intl: MdSortIntl,
+              @Optional() public _cdkColumnDef: CdkColumnDef) {


Put the @Optional args next to each other?

jelbourn · 2017-06-23T03:30:59Z

src/lib/sort/sort-header.ts

+
+  /** Whether this MdSortHeader is currently sorted in either ascending or descending order. */
+  _isSorted() {
+    return this._sort.active == this.id && this._sort.direction != '';


Just this._sort.active == this.id && this._sort.direction?

jelbourn · 2017-06-23T03:38:03Z

src/lib/sort/sort.ts

+  /**
+   * Register function to be used by the contained MdSortables. Adds the MdSortable to the
+   * collection of MdSortables.
+   * @docs-private


Why @docs-private here?

Hmm I wasn't thinking others needed it but people implementing their own MdSortable certainly would. Removed

jelbourn · 2017-06-23T21:51:36Z

src/lib/sort/sort-header.html

@@ -0,0 +1,20 @@
+<div class="mat-sort-header-container" type="button"
+     [class.mat-sort-header-position-before]="arrowPosition == 'before'">
+  <button class="mat-sort-header-button"


Still needs type="button"

Ah misunderstood, thought this was an accessibility thing, putting type on the wrong element. Good now

jelbourn · 2017-06-23T22:44:32Z

src/lib/sort/sort-header.ts

+  host: {
+    '(click)': '_sort.sort(this)',
+    '[class.mat-sort-header-sorted]': '_isSorted()',
+  },


Still missing view encapsulation none and onpush

Whoops, got removed in testing. It's back in

andrewseguin · 2017-06-23T23:27:15Z

Good for another review round

jelbourn · 2017-06-23T23:41:39Z

src/lib/sort/sort-direction.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+export type SortDirection = 'ascending' | 'descending' | '';


Maybe just asc and desc?

jelbourn · 2017-06-24T12:57:54Z

src/lib/sort/sort.ts

+    if (!sortable) { return ''; }
+
+    // Get the sort direction cycle with the potential sortable overrides.
+    const disableClear = sortable.disableClear != undefined ?


Prefer doing == null or != null (it captures both null and undefined and is shorter)

jelbourn · 2017-06-24T13:00:00Z

src/lib/sort/sort-header.ts

+      throw getMdSortHeaderNotContainedWithinMdSortError();
+    }
+
+    _sort.mdSortChange.subscribe(() => _changeDetectorRef.markForCheck());


Need to unsubscribe to this on destroy

jelbourn · 2017-06-24T13:01:01Z

src/lib/sort/sort-header.ts

+  }
+
+  ngOnDestroy() {
+    this._sort.unregister(this);


nit: deregister

jelbourn · 2017-06-24T13:01:35Z

src/lib/sort/sort.spec.ts

+      ],
+    }).compileComponents();
+
+    fixture = TestBed.createComponent(SimpleMdSortApp);


createComponent has to go in a separate beforeEach block because compileComponents() is asynchronous

andrewseguin · 2017-06-26T18:22:32Z

Made the changes, thanks

jelbourn · 2017-06-26T18:31:49Z

src/demo-app/data-table/person-data-source.ts

+      let valueA = isNaN(+propertyA) ? propertyA : +propertyA;
+      let valueB = isNaN(+propertyB) ? propertyB : +propertyB;
+
+      return (valueA < valueB ? -1 : 1) * (this._sort.direction == 'ascending' ? 1 : -1);


Missed a spot

jelbourn · 2017-06-26T18:32:03Z

src/demo-app/data-table/data-table-demo.html

      <cdk-cell *cdkCellDef="let row"> {{row.id}} </cdk-cell>
    </ng-container>

    <!-- Column Definition: Progress -->
    <ng-container cdkColumnDef="progress">
-      <cdk-header-cell *cdkHeaderCellDef> Progress </cdk-header-cell>
+      <cdk-header-cell *cdkHeaderCellDef
+                       md-sort-header start="descending">


andrewseguin · 2017-06-26T19:59:54Z

D'oh, forgot to export my last fixes - should be good to go

jelbourn

LGTM

jelbourn · 2017-06-26T20:19:52Z

Caretaker note: this will need a new build rule

ErinCoughlan

LGTM

ErinCoughlan · 2017-06-26T21:10:22Z

src/lib/sort/sort-header-intl.ts

+
+  /** A label to describe the current sort (visible only to screenreaders). */
+  sortDescriptionLabel = (id: string, direction: SortDirection) => {
+    return `Sorted by ${id} ${direction}`;


If direction is 'asc' | 'desc', won't that sound strange with a screenreader? I wonder if you want to override it to be a real word, but I'm not super familiar with what the standards are here.

Good catch, I'll add a check for this

* feat(sort): add sortable * checkin * checkin * feat(sort): add sort header * overrides, tests * format demo html * add ngif to screenready label * add new line to scss * fix tests * fix types * fix types * shorten coerce import * comments * comments * rebase * specialize intl to header; make public * remove reverse * button type and onpush * rename sort directions (shorten) * small changes * remove consolelog * screenreader

angular-automatic-lock-bot · 2019-09-07T20:01:51Z

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

andrewseguin added the pr: needs review label Jun 22, 2017

andrewseguin requested review from DerekLouie, jelbourn and ErinCoughlan June 22, 2017 19:08

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

andrewseguin force-pushed the sortable branch from 11d38a6 to dc5d97e Compare June 22, 2017 23:12

jelbourn reviewed Jun 23, 2017

View reviewed changes

andrewseguin force-pushed the sortable branch from 2a7ec52 to 629848c Compare June 23, 2017 18:38

andrewseguin added 15 commits June 23, 2017 13:45

feat(sort): add sortable

12ec1fb

checkin

4eea9a6

checkin

c5244b3

feat(sort): add sort header

af0df28

overrides, tests

29ef808

format demo html

45a06af

add ngif to screenready label

1644763

add new line to scss

06adfc7

fix tests

9cee02b

fix types

efe57aa

fix types

d3ff9f8

shorten coerce import

cf272ce

comments

1684996

comments

9240dd3

rebase

7b32a05

andrewseguin force-pushed the sortable branch from 629848c to 7b32a05 Compare June 23, 2017 20:58

specialize intl to header; make public

3341fee

jelbourn reviewed Jun 23, 2017

View reviewed changes

andrewseguin added 2 commits June 23, 2017 16:24

remove reverse

6e11c41

button type and onpush

25b4fd6

jelbourn reviewed Jun 24, 2017

View reviewed changes

andrewseguin added 3 commits June 26, 2017 10:35

rename sort directions (shorten)

ec18696

small changes

cdeba41

remove consolelog

eb3556e

jelbourn reviewed Jun 26, 2017

View reviewed changes

jelbourn approved these changes Jun 26, 2017

View reviewed changes

jelbourn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jun 26, 2017

ErinCoughlan approved these changes Jun 26, 2017

View reviewed changes

screenreader

ea51206

tinayuangao merged commit b328d36 into angular:master Jun 27, 2017

andrewseguin deleted the sortable branch November 28, 2017 20:36

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

feat(sort): add ability to manage and display sorting #5307

feat(sort): add ability to manage and display sorting #5307

Conversation

andrewseguin commented Jun 22, 2017 • edited

jelbourn 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

jelbourn Jun 23, 2017 • edited

Choose a reason for hiding this comment

jelbourn Jun 23, 2017 • edited

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

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

andrewseguin commented Jun 23, 2017

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

andrewseguin commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Jun 26, 2017

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn commented Jun 26, 2017

ErinCoughlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

andrewseguin commented Jun 22, 2017 •

edited

jelbourn Jun 23, 2017 •

edited

jelbourn Jun 23, 2017 •

edited