docs(table): change docs to use MatTableDataSource #8007

andrewseguin · 2017-10-24T20:24:24Z

Drastically simplifies the examples

willshowell

Only minor nits.

It's hard to tell, but this removes all custom data source examples, right? Would it be valuable in a follow up PR to demo implementing your own data source?

willshowell · 2017-10-25T15:44:55Z

src/material-examples/table-filtering/table-filtering-example.ts

-        });
+  applyFilter(filterValue: string) {
+    filterValue = filterValue.trim(); // Remove whitespace
+    filterValue = filterValue.toLowerCase(); // Datasource defaults to lowercase matches


// MatTableDataSource defaults to lowercase matches?

willshowell · 2017-10-25T15:47:26Z

src/material-examples/table-http/table-http-example.ts

+    // If the user changes the sort order, reset back to the first page.
+    this.sort.sortChange.subscribe(() => this.paginator.pageIndex = 0);
+
+    Observable.merge(...[this.sort.sortChange, this.paginator.page])


It may be simpler to just do,

Observable.merge(this.sort.sortChange, this.paginator.page)

since you're no longer defining displayDataChanges

willshowell · 2017-10-25T15:50:43Z

src/material-examples/table-http/table-http-example.ts

+          this.resultsLength = data.total_count
+
+          return data.items;
+        })


Might be more idiomatic to place inside do? Or would that just end up more confusing 🤷‍♂️?

.do(data => { // Flip flag to show that loading has finished. this.isLoadingResults = false; this.isRateLimitReached = false; this.resultsLength = data.total_count; }) .map(data => data.items)

I like do better here but concerned that adding more operators will distract people. Hard balance to find since people will probably copy/paste this =P

Yeah, agreed on it being distracting. map works just as well since you still do have to map it to items.

willshowell · 2017-10-25T16:23:23Z

src/material-examples/table-overview/table-overview-example.ts

+  /**
+   * Set the paginator and sort after the view has been initialized so
+   * that the components are completely set up for the data source to access.
+   */


Is there any other way to word this? "...so that the components are completely set up..." kind of glosses over why it has to be here instead of in, say, OnInit.

Maybe

Set the paginator and sort after the view has been initialized since that is
the first time the component has access to the ViewChild queries.

Changed to

Set the sort after the view init since this component will be able to query its view for the initialized sort.

Think that's alright?

Wait until after the view init to set the paginator and sort in the data source. This will enable the component to query its view for the initialized paginator and sort components.

Maybe an improvement? I guess it doesn't really matter, and I think having any comment at all should signify that it's important to keep it there after the copy-paste.

andrewseguin · 2017-10-25T17:24:06Z

Made the suggested changes, thanks for the thorough review! I'm currently in the process of revamping the docs, and I'll share the doc with you for suggestions on what should be included (like a custom data source)

jelbourn · 2017-10-26T05:03:56Z

@ErinCoughlan how do these example changes look to you?

chrste90 · 2017-10-26T06:41:40Z

src/material-examples/table-http/table-http-example.ts


-    return this._httpClient.get<GithubApi>(requestUrl);
+    return this.http.get(requestUrl)
+        .map(response => response as GithubApi);


Maybe it's nicer to use the type from HttpClient? this.http.get<GithubApi>(requestUrl)

ErinCoughlan · 2017-10-26T16:44:34Z

src/material-examples/table-http/table-http-example.css

+
+.mat-column-created {
+  max-width: 124px;
+}


no newline at end of file

ErinCoughlan · 2017-10-26T16:49:34Z

src/material-examples/table-overview/table-overview-example.ts

-      this.exampleDatabase.data.forEach(data => this.selection.select(data.id));
-    }
+  applyFilter(filterValue: string) {
+    filterValue = filterValue.trim(); // Remove whitespace


Does it make sense to add the trim and toLowerCase into the MatTableDataSource? It's not intuitive as a user that either of these would be necessary.

I think so - I expect most people will want it trimmed and lowercase by default. I'll make another PR that does this.

PR #8059 created to address this - data source's entire filter matching function can be overriden and by default it will lowercase/trim

ErinCoughlan · 2017-10-26T16:50:18Z

src/material-examples/table-sorting/table-sorting-example.css

  color: black;
-}
+}


No newline at the end of the file.

ErinCoughlan · 2017-10-26T16:51:56Z

@jelbourn These seem like better examples and overall less complicated.

jelbourn

LGTM

jelbourn · 2017-10-26T17:07:50Z

Add merge-ready when ready

wk-done · 2017-10-31T07:36:45Z

Hi, I noticed that the selection of rows was removed from the examples (table-overview-example) with these changes.
If I'm not completely wrong it was the only example out there how to use selection in the data-table. Is there an other example page or will there be one in the future?

angular-automatic-lock-bot · 2019-09-07T20:02:38Z

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

chore(table): change docs to use MatTableDataSource

6006853

andrewseguin added P2 The issue is important to a large percentage of users, with a workaround pr: needs review labels Oct 24, 2017

andrewseguin requested a review from jelbourn October 24, 2017 20:24

andrewseguin requested a review from amcdnl as a code owner October 24, 2017 20:24

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

andrewseguin changed the title ~~chore(table): change docs to use MatTableDataSource~~ docs(table): change docs to use MatTableDataSource Oct 24, 2017

willshowell reviewed Oct 25, 2017

View reviewed changes

review changes

320dcbd

chrste90 reviewed Oct 26, 2017

View reviewed changes

ErinCoughlan reviewed Oct 26, 2017

View reviewed changes

ErinCoughlan approved these changes Oct 26, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Oct 26, 2017

jelbourn approved these changes Oct 26, 2017

View reviewed changes

jelbourn added the pr: merge safe label Oct 26, 2017

comments

337ba5c

andrewseguin added the action: merge The PR is ready for merge by the caretaker label Oct 26, 2017

mmalerba merged commit e15fc0b into angular:master Oct 27, 2017

andrewseguin deleted the table-update-examples branch November 28, 2017 20:39

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

docs(table): change docs to use MatTableDataSource #8007

docs(table): change docs to use MatTableDataSource #8007

Uh oh!

Conversation

andrewseguin commented Oct 24, 2017

Uh oh!

willshowell 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!

willshowell Oct 25, 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!

andrewseguin Oct 25, 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!

andrewseguin commented Oct 25, 2017

Uh oh!

jelbourn commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan Oct 26, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan commented Oct 26, 2017

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Oct 26, 2017

Uh oh!

wk-done commented Oct 31, 2017

Uh oh!

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

Uh oh!

Uh oh!

willshowell Oct 25, 2017 •

edited

Loading

andrewseguin Oct 25, 2017 •

edited

Loading

ErinCoughlan Oct 26, 2017 •

edited

Loading