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

DataSource: encapsulate displayColumns #5883

Closed
marshall007 opened this issue Jul 19, 2017 · 8 comments
Closed

DataSource: encapsulate displayColumns #5883

marshall007 opened this issue Jul 19, 2017 · 8 comments
Assignees
Labels
needs: discussion Further discussion with the team is needed before proceeding

Comments

@marshall007
Copy link
Contributor

Bug, feature request, or proposal:

Proposal

What is the expected behavior?

I would like to propose that the DataSource class be used to encapsulate the columns parameter in *cdkHeaderRowDef="columns" and *cdkRowDef="let row; columns: columns".

I haven't been able to find any documentation on the rationale behind the current DataSource implementation, but based on the usage examples, it seems like general purpose implementations of DataSource would benefit from having access to the column definitions.

What is the current behavior?

Current behavior is that displayColumns is a property of the parent component and is not accessible to the DataSource.

What is the use-case or motivation for changing an existing behavior?

Given that the docs appear to encourage implementing filter functionality within DataSource, you could imagine a generic FilteredDataSource that only applies search criteria to the visible columns.

As mentioned above, a nice side-effect would be simplifying usage of md-header-row and md-row by having fewer required inputs.

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

Material v2.0.0-beta.8+.

Is there anything else we should know?

If nothing else it would be nice to get better documentation on the rationale behind DataSource and its intended use. Currently it doesn't seem to do enough to justify itself in the simple case. Better examples would also be helpful (#5848).

@andrewseguin
Copy link
Contributor

andrewseguin commented Jul 19, 2017

The DataSource is a general data provider solution for any component that will want to be connected with a CollectionViewer, which is any component that requests an array data stream for rendering. Right now our only example of a CollectionViewer is the table, but in the future we will also use this class with components such as the <md-tree>.

The benefit to the class is that we created an API that can be responsive to changes in the view (e.g. the user has scrolled and we can provide information on what rows are now viewed) as well as providing a hook such that the user can trigger rendering updates.

With respect to why we didn't include displayedColumns in the data source, there are two major reasons. First, the DataSource will not be uniquely used for the table and adding the idea of columns to it will specialize it too much. Second, we want to be able to implement features such as sections and conditional rows into the table, which will require headers and rows to know what columns they should show.

I certainly agree that as of now, it would be a lot nicer API if columns were held by the DataSource or even just an input to the table itself, but we wanted to make sure that we designed an open interface that gave us enough flexibility to incorporate features that may depend on more dynamic rows/headers/columns.

If you're interested further into the design of the table and some of its rationale, please check out its design doc: https://docs.google.com/document/d/1ZyKhwrgqfTBAn7saTq2jPlep2_CwSw5DeoZ8UbaXrC0

@marshall007
Copy link
Contributor Author

marshall007 commented Jul 20, 2017

@andrewseguin that all makes perfect sense as to why you'd want to keep the base DataSource interface generic and thanks for linking to the design doc!

That being said, I'm thinking it would still make sense to expose something like TableDataSource extending the base DataSource implementation and adding support for display columns. I do not see "more dynamic behavior" for tables and "encapsulate columns" as mutually exclusive. If anything, it seems like encapsulating the display columns in a TableDataSource would almost be a prerequisite to enabling more dynamic behavior.

I mentioned the FilteredDataSource use case above, but arguably an even more compelling case would be to inform the DataSource what fields it should bother fetching from a remote resource.

@andrewseguin
Copy link
Contributor

I could see a benefit in some use cases where the DataSource knows which columns are being displayed, especially if it helps with data requests.

I'm not sure how this would reconcile with features such as sections (#5891 - consecutive sets of data with different columns and headers) or conditional rows (#5887 - the rows and event/attribute bindings are conditional on the row data). In these features, the rows themselves must understand what cells should be rendered and the DataSource would not be able to differentiate which rows want which columns.

I could imagine that as we develop sections, that the input for columns will only need to go to the section rather than to both the header row and data row templates, except conditional rows would still want to be able to override this.

@marshall007
Copy link
Contributor Author

marshall007 commented Jul 20, 2017

@andrewseguin my initial impression for #5891 would be that you'd have some new parent wrapper component like md-table-group which would then contain one or more md-table, each with their own corresponding DataSource. At least at first glance, this would make a lot more sense to me than trying to shoehorn multiple disparate data sets/sources into a single DataSource/md-table definition.

[edit]: it also seems likely that you'd probably want the ability to connect() / disconnect() each data source in an md-table-group as it scrolls into and out of view. If this is implemented using just a single DataSource then it's no longer providing a useful abstraction for connecting data sources to each table section.

Without thinking about it in too much detail, I don't have a great answer for what this would mean concerning #5887. My first thought is that TableDataSource could expose getColumnsFor(row), which would be called for each (visible) md-row definition. The base implementation would just return this.displayColumns, but could be overridden if you need to customize that behavior.

@jelbourn
Copy link
Member

The split between the data source and the table exists to separate two fundamentally different responsibilities: what is the data, and how is the data displayed. The columns being displayed falls into the "display" category, and moving it to the data source would go against this separation of responsibilities.

@marshall007
Copy link
Contributor Author

The columns being displayed falls into the "display" category, and moving it to the data source would go against this separation of responsibilities.

@jelbourn I think we're drawing somewhat arbitrary lines in the sand here while disregarding the implications to usability.

I've already provided two example cases demonstrating when these responsibilities are in fact inseparable. This is especially true when we start talking about data-driven or otherwise dynamic columns. I might choose to load the display columns from a remote resource, local storage, etc, all of which might further be customized depending on row context. If we don't start encapsulating this functionality somewhere, components using md-table are going to balloon in complexity with no ability to harness generic implementations.

I don't disagree that DataSource is a useful primitive. What I'm saying is that in order for it to really be useful in the context of the target component(s), it needs to be extended to encapsulate more "view" logic and the cross-cutting concerns between "view" and "data" of that component.

I would imagine we'll run into many of the same issues with md-tree, which would similarly warrant a TreeDataSource. You can imagine wanting to configure things like lazy loading nodes and how deeply to initially fetch.

If we want to talk about editable md-table and md-tree implementations, you'll want hooks for when rows/nodes are modified and created. You'd also want hooks for when paginated data is about to be unloaded or fetched to warn the user of unsaved changes.

I think at the very least this issue warrants more discussion.

@jelbourn
Copy link
Member

The distinction isn't arbitrary. The question of "where does data come from" is genuinely a separate concern from "how is data rendered".

The primary intent behind the idea of the data source is to leave all of the specifics about "where does data come from" to the user. This prevents us from doing anything with the data source that doesn't mesh well with any of the myriad of data sources out there; the only behavior baked into the data source itself is that which you literally couldn't render rows of cells without.

We expect people to extend DataSource with their specific logic. For example, if you want to have your data fetching influenced by the visible columns, we would consider that to be a behavior specific to your application that would go in your version of the DataSource.

Another aspect to this is that a future enhancement to the table will be the addition of multiple row templates, each of which can have a different set of columns.

For tree there actually is a TreeDataSource, which deals with how the hierarchy is defined, since the hierarchy is inherently necessary for rendering a hierarchical display (we should probably call it HierarchicalDataSource).

@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 6, 2019
@mmalerba mmalerba added the needs: discussion Further discussion with the team is needed before proceeding label Mar 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: discussion Further discussion with the team is needed before proceeding
Projects
None yet
Development

No branches or pull requests

4 participants