Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(table): allow data input to be array, stream #9489

Merged
merged 5 commits into from
Feb 1, 2018

Conversation

andrewseguin
Copy link
Contributor

@andrewseguin andrewseguin commented Jan 20, 2018

Removes requirement of implementing/using a DataSource to use the table. Instead, users can directly provide a data array or stream of data arrays directly as the data source.

@willshowell Can you take a look, especially the revamped docs?

Closes #8227

@andrewseguin andrewseguin added P2 The issue is important to a large percentage of users, with a workaround pr: needs review labels Jan 20, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 20, 2018
Copy link
Contributor

@willshowell willshowell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm excited for this! I think it'll reduce a lot of first-try friction.

expectedRender.push(['a_4', 'b_4', 'c_4']);
stream.next(data);
dataInputFixture.detectChanges();
expectTableToMatchContent(dataInputTableElement, expectedRender);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there value in also testing the expected behavior around table.renderRows() with the stream input?

Particularly, I'm curious what the expected behavior is after

data.push({a: 'a_4', b: 'b_4', c: 'c_4'});
// Note: not advancing BehaviorSubject
dataInputComponent.table.renderRows();
dataInputFixture.detectChanges();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, I do think there's value in making sure that the renderRows will work like this. I expect they'll be a subset of users who use a stream to provide some array but would like to still manipulate the array and not think about emitting again

* a header row and data rows. Updates the rows when new data is provided by the data source.
* A data table that renders a header row and data rows. Uses the dataSource input to determine
* the data to be rendered. The data can be provided either as a data array, a stream that emits
* the data array to render, or a DataSource that provides a stream that emits the data array to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider replacing 'stream' with 'Observable' or 'Observable stream' here and throughout, for clarity. Though, 'stream' may be self explanatory given the dataSource property will show Observable<T[]> in the docs.

* Sets the table's source of data, which can be provided in three ways (in order of complexity):
* - Simple data array (each object represents one table row)
* - Stream that emits a data array each time the array changes
* - `DataSource` object that implements the connect/disconnect interface.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit: inconsistent periods

input. The table will take the array and render a row for each object in the data array.

```html
<mat-table [data]=”myDataArray”>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dataSource here and above? Or was an alias added?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it should be dataSource

array. Instead, when objects are added, removed, or moved on the data array, you can trigger an
update to the table's rendered rows by calling its `renderRows()` function.

See the section "Alternative ways to provide data" for more approaches to providing data to the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a link so the toc will pick it up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they'll be a link we can use but not sure if we've done that yet in our docs. We should make sure that's supported or easy to do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TOC will link to section headers, but any links in the content have to be manually added.

I would also rephrase this paragraph to something like

While an array is the _simplest_ way to bind data into the data source, it is also
the most limited. For more complex applications, using a `DataSource` instance
is recommended. See xxx below for more information.


Also, another approach to providing data is by implementing a `DataSource`. This is simply
an interface that has two functions: `connect` and `disconnect`. After passing a DataSource to the
table, the table will call `connect()` to receive a stream that emits the data array that should be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: consistent use of parenthesis for methods

include/exclude columns dynamically.

### Alternative ways to provide data

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a little blurb to refresh the reader of what the original option is

Alternative ways to provide data

The simplest way to provide data to your table is by passing a data array. More complex use-cases may benefit from a more flexible approach. (or something like that)

Observable stream of data arrays

...

DataSource

...

to clean up any subscriptions that may have been registered during the `connect` process.

The benefit to creating a `DataSource` class is that it can decouple rendering logic from your
component and keep it encapsulated in a separate class (e.g. sorting, filtering, pagination, e.g.).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to eventually have an example for each of these that is just like table-basic, but using the alternative approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 I think this would be useful to demonstrate the differences, I'll make an issue following this PR getting merged

@andrewseguin
Copy link
Contributor Author

I'm excited as well, I think this will help make things a lot easier and reduce the learning curve of the table. Next for me is to add some kind of mat-simple-column (like in the demos) so the learning curve of the templates are reduced too

@@ -153,17 +157,32 @@ export class CdkTable<T> implements CollectionViewer, OnInit, AfterContentChecke
private _trackByFn: TrackByFunction<T>;

/**
* Provides a stream containing the latest data array to render. Influenced by the table's
* stream of view window (what rows are currently on screen).
* Sets the table's source of data, which can be provided in three ways (in order of complexity):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omit "Sets", since this is a property, not a method

*
* If a data array is provided, the table must be notified when the array's objects are
* added, removed, or moved. This can be done by calling the `renderRows()` function which will
* render the diff since the last table render.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about when the array reference changes?

* render the diff since the last table render.
*
* When providing an Observable stream, the table will trigger an update automatically when the
* stream emits a new array of data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omit "a new array of data"?

* directly, you will need to call this function whenever data in the provided array is
* added/removed/moved in-place.
*/
renderRows() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's your thinking on renderRows vs. refresh or render?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion really, I'm open to any of those. Maybe refresh sounds more approachable for users to call?

* Checks for differences in the data since the last diff to perform only the necessary
* changes (add/remove/move rows).
*
* If the table's data source is a DataSource or Observable, this will be invoked each time the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"invoked automatically"

array. Instead, when objects are added, removed, or moved on the data array, you can trigger an
update to the table's rendered rows by calling its `renderRows()` function.

See the section "Alternative ways to provide data" for more approaches to providing data to the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TOC will link to section headers, but any links in the content have to be manually added.

I would also rephrase this paragraph to something like

While an array is the _simplest_ way to bind data into the data source, it is also
the most limited. For more complex applications, using a `DataSource` instance
is recommended. See xxx below for more information.


Start by writing your table's column definitions. Each column definition should be given a unique
name and contain the content for its header and row cells.
#### 2. Define the table's columns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to say "column templates" (and "row templates" below)? Having "Define the table's rows" feels weird since the data drives the rows, you're just specifying the bindings.

This means that by changing your column list provided to the rows, you can easily re-order and
include/exclude columns dynamically.

### Alternative ways to provide data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say "Advanced data sources"


#### DataSource

Also, another approach to providing data is by implementing a `DataSource`. This is simply
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change this first sentence to something like

For most real-world applications, providing the table a `DataSource` instance will be the best way to manage data.

It's also a base class, not an interface. I would put the "Why use a data source" bits before the "what is it" bits; we should say something like "The DataSource is meant to serve a place to encapsulate any sorting, filtering, pagination, and data retrieval logic specific to the application."

mutating the data provided to the table according to their outputs.

To simplify the use case of having a table that can sort, paginate, and filter an array of data,
the Angular Material library comes with a `MatTableDataSource` that has already implemented
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still want to rename this to ClientArrayDataSource (or ClientDataSource or ArrayDataSource or even SimpleDataSource)

My problem with the current name is that it implies that it is the way to provide data for a <mat-table>, wich may lead people to think that the table doesn't support server side sorting/pagination/filtering, or even just other kinds of filtering than search.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'm open to renaming. We should brainstorm offline to figure out the best name

@andrewseguin andrewseguin force-pushed the table-data-array-input branch 10 times, most recently from 179b9e3 to 018735f Compare January 30, 2018 18:28
@andrewseguin
Copy link
Contributor Author

Ready for another round of review

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments still seem unresolved

@andrewseguin
Copy link
Contributor Author

Alright, mostly sure now that it's good. Had an odd time merging conflicts

@jelbourn jelbourn added the target: minor This PR is targeted for the next minor release label Jan 30, 2018
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jan 30, 2018
@tinayuangao tinayuangao merged commit 085d805 into angular:master Feb 1, 2018
mmalerba pushed a commit to mmalerba/components that referenced this pull request Feb 1, 2018
* feat(table): allow data input to be array, stream

* comments

* fix connect function

* fix connect function v2

* fix demo imports
@andrewseguin andrewseguin deleted the table-data-array-input branch April 17, 2018 17:23
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add detailed documentation for using DataSource with MatTable
5 participants