Skip to content

feat(tree): make data source accept array or observable #9847

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 13, 2018
Merged

feat(tree): make data source accept array or observable #9847

merged 1 commit into from
Feb 13, 2018

Conversation

tinayuangao
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 8, 2018
jelbourn
jelbourn reviewed Feb 9, 2018
View reviewed changes
src/cdk-experimental/tree/tree.ts Outdated
this._data = new Array<T>();

if (this.dataSource) {
if (this.dataSource instanceof DataSource) {
Copy link
Member

Choose a reason for hiding this comment

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

We can into problems with the table using instanceof for this since people often create fakes for tests that are just object literals with a connect method. @andrewseguin what did you end up doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Using the same way as table now. Please take a look. Thanks!

src/cdk-experimental/tree/tree.ts Outdated
dataStream = this._dataSource.connect(this);
} else if (this._dataSource instanceof Observable) {
dataStream = this._dataSource;
} else if (this._dataSource instanceof Array) {
Copy link
Member

Choose a reason for hiding this comment

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

Use Array.isArray(...)

@tinayuangao tinayuangao force-pushed the tree-datasource2 branch 2 times, most recently from 31ab6d2 to 7056d38 Compare February 9, 2018 23:23
jelbourn
jelbourn reviewed Feb 13, 2018
View reviewed changes
src/cdk/tree/tree.ts Outdated

// Cannot use `instanceof DataSource` since the data source could be a literal with
// `connect` function and may not extends DataSource.
if ((this._dataSource as DataSource<T>).connect instanceof Function) {
Copy link
Member

Choose a reason for hiding this comment

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

I missed this in the table PR, but it should use typeof rather than instanceof since the former is generally more reliable for primitive types.

Also it should use the same checks for the instanceof DataSource checks in the rest of the file (which I also missed in the table PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Thanks for review!

jelbourn
jelbourn approved these changes Feb 13, 2018
View reviewed changes
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 labels Feb 13, 2018
@tinayuangao tinayuangao merged commit f894cdf into angular:tree Feb 13, 2018
tinayuangao added a commit that referenced this pull request Feb 13, 2018
@tinayuangao
feat(tree): make data source accept array or observable (#9847)
b519039
@andrewseguin andrewseguin added the target: minor This PR is targeted for the next minor release label Feb 20, 2018
@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.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tinayuangao @jelbourn @googlebot @andrewseguin