Skip to content
This repository has been archived by the owner on Feb 2, 2019. It is now read-only.

feat: Data table component integration #117

Merged
merged 1 commit into from
Mar 27, 2016

Conversation

Gregcop1
Copy link
Collaborator

Here is a try of what data table component could look alike.
ATM, I've only worked on the selectable rows but we can imagine building styles for sortable column, truncate header and all other recommandations in Material Design

@Gregcop1 Gregcop1 force-pushed the feature/data-table branch from 1359e0a to c746883 Compare March 18, 2016 15:14
@justindujardin
Copy link
Owner

@Gregcop1 this is a really cool start. Can you please bring it up to date with the latest master build, and use (2) spaces for your indentation of the component and its HTML examples.

import {MdCheckbox} from '../checkbox/checkbox';

/**
* @name mdDataTable
Copy link
Owner

Choose a reason for hiding this comment

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

We don't parse the docs like the angular/material repo does. I think this markdown documentation would be a great fit for being put in examples/components/data_table/readme.md file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@Gregcop1 Gregcop1 force-pushed the feature/data-table branch from bf5c831 to 51c45f4 Compare March 21, 2016 13:18
</div>


<div flex="100" layout="column" flex-xs="100" layout-align="left start">
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry that I was unclear. I'd prefer the usage documentation to be in the readme.md file like the material2 team is doing. Please see this as an example: https://github.com/angular/material2/blob/master/src/components/sidenav/README.md

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem. I will change it but at the moment, you take the content of readme file and insert it in the example/app and the render is not so pretty :)

@Gregcop1 Gregcop1 force-pushed the feature/data-table branch from 51c45f4 to 7923f51 Compare March 21, 2016 23:56
@Gregcop1
Copy link
Collaborator Author

I've deported the doc to a readme file in the component directory and rename the non-numeric class.
Test are incoming.
Travis continue to fail. Do you have any idea about that ?

@joshwiens
Copy link

screen shot 2016-03-21 at 7 07 27 pm

@Gregcop1 Gregcop1 force-pushed the feature/data-table branch from 7923f51 to 23edd90 Compare March 22, 2016 08:15
@Gregcop1
Copy link
Collaborator Author

And here come the tests. I will check later to fix travis

@Gregcop1
Copy link
Collaborator Author

Issue of dependency to RXJS (which broke Travis) is on master too so it's not related to this PR. We have to fix it first.

@ollwenjones
Copy link
Collaborator

Looks like I am too slow... Was down to unit testing the data-table in my fork and got pulled off onto something else. Always bummed about duplicated work, but hopefully it won't be all wasted...

@ollwenjones
Copy link
Collaborator

I think what I have implemented is a little more robust: column models for (eventually) showing/hiding columns, sorting by columns(already). Also the table component takes a columns model and wraps cell templates, so it's composeable, but the implementer is abstracted from the table html elements. Of course the CSS class-names still work for any table-tag component anyone else wants to roll.

I'd be interested to hear what you think @justindujardin & @Gregcop1
example component, example template

I guess the work isn't wasted, but now there's something to merge with, and others aren't stuck waiting on me to have something. 😉

@Gregcop1
Copy link
Collaborator Author

Nice work :) I agree it's a way more robust event if I found it a bit complicated. It's really far from conventional table declaration. In my opinion Material Design Component should be easy to integrate and should stick more or less the component which is enhanced.

I may be wrong but with your method, it seems that I can't add my own class or other behavior on a TH or a TD cause you build them on your own. How can I add a icon on the header of a column ? How can I add an edit icon on each line or enable inline editing ? You see what I mean ?

It's only an opinion but I think we have to add features and stay as transparent as possible.

@ollwenjones
Copy link
Collaborator

@Gregcop1 Thank you very much for taking the time to review and provide feedback.

It is a bit complicated because of some of the goals for future features, and also the desire to DRY up / abstract as much table boilerplate away as possible. Related to that, regarding your comment that a Material Design component "should stick more or less the component which is enhanced": That could be true, but I've never read that before. I thought Material Design was primarily a graphic design/UX pattern that had nothing to do with implementation.

Very fair point about customizing TH tags. Maybe I took that too far. I thought column headings would be an easy thing to encapsulate, because for tracking things like showing/hiding columns we'll need a column model anyway. The counterpoint is that as I understand the the Material Design spec for Data Tables, those are not things people should be doing anyway. Table controls should be in a toolbar above the table: http://www.google.com/design/spec/components/data-tables.html#data-tables-tables-within-cards

If my interpretation is wrong, then I'll need to provide templates for columns the same way I've provided templates for cells, or else fall back to an approach more like yours.

My main reasons for abstracting away the table HTML were:

  1. It's less boilerplate for the implementer deal with.
  2. Encapsulating some structures allows enforcement of MD guide.
  3. The implementation details are abstracted away, so if the underlying HTML needs to change to support future features, nobody's implementation has to change.
  4. The Java developers around me will be happier not mucking with the table HTML. 😉

I / We probably do need to accommodate both approaches via directives or meet in the middle somehow.

Thanks much again for the time and feedback!

@joshwiens
Copy link

Has anyone approached @daniel-nagy about a Angular2 version of the md-data-table project?

There has been a significant amount of community discussion around it in his project already which would prove valuable here imo.

//cc @justindujardin @ollwenjones @Gregcop1

@ollwenjones
Copy link
Collaborator

Forgot to mention

I may be wrong but with your method, it seems that I can't add my own class or other behavior on a TH or a TD

I'm only abstracting away the THs - and while the component draws the actual TD tags, those cell templates provide full control over what expression or component the user puts inside each one.

@ollwenjones
Copy link
Collaborator

@d3viant0ne I haven't. Afraid I wasn't aware of it... Maybe I subconsciously avoided referencing Angular 1.x work since 2.x is so different.

@joshwiens
Copy link

@ollwenjones - iirc he was considering doing an angular2 version. Getting that to land here would be a boost for the project assuming it works out. Also something that would clearly differentiate ng2-material form the Google repo as I'm betting they punt on a data table again.

Just my $0.02

@ollwenjones
Copy link
Collaborator

Data Table is a firm project requirement for me, so it'll have to live here or back in-house, I guess. ;)

@Gregcop1 - initially I had template tags on a TR level, instead of a TD level... I wonder if I would have to go back at least that far to handle things like expandable sub-rows etc. ?

@ollwenjones
Copy link
Collaborator

@d3viant0ne I almost can't blame them for punting... data-tables are a complex wheel (like autocompletes) that so many are sick of reinventing.

@joshwiens
Copy link

Yeah, I didn't gripe about it as I completely understand the complexity, particularly when trying to make someone's gigantic data table responsive and not vomit when viewed on a device.

That said, displaying data is at the core of most web applications. If you want to make a front end framework you really don't have a choice but to add a data table component.

@Gregcop1
Copy link
Collaborator Author

Oh nice @d3viant0ne I haven't search for the ng-data-table in angular 1. I will be glad to discuss with him why he goes so far :) Pagination and extras is more than Material Design recommandation about table. It could be complementary components in my opinion.
I've launch a question about md-data-table in the ng-material2 project too to see if they want something like this or not. Their project is in alpha now so I don't where is the best place to contribute. //cc @justindujardin

In my opinion Material Design Component should be easy to integrate and should stick more or less the component which is enhanced.
@ollwenjones It was only my opinion, not a recommandation. I prefer light, open and compatibles component that an overkill one. That's why I thought too that md-data-table component goes a little too far (with pagination and extras)
hiding column is about responsive design not material design. You can add already add this feature with Media for example.

@ollwenjones
Copy link
Collaborator

@Gregcop1 I can think of other use cases for hiding columns besides responsive design: Desktop user wants to customize his/her table view for the task he/she is doing. - A real live use case in my project, which would be supported by the columns model.

@ollwenjones
Copy link
Collaborator

I agree with you about "overkill one"s - definitely seen that in the wild. Mulitple components for different features with compose-ability is the way to go - just not sure that means writing table > thead > tr > th by hand every time. 😄

@Gregcop1
Copy link
Collaborator Author

which is not related to Material Design ;)
For me a md-data-table component is a basis that you can override with other directives on demand

@daniel-nagy
Copy link

I could rollout an Angular2 version of my current project. It may be a couple weeks or more till I get around to it.

@jraadt
Copy link

jraadt commented Mar 24, 2016

My team has analyzed many data table implementations for angular 1 and 2 that adhere to material specifications. We not only examine how the UI looks, but also how it's implemented. The best we found was @daniel-nagy md-data-table. It's pretty incredible what he's put together. Feature rich (including inline editing) that really follows material look & feel and best practices.

I'm excited to hear he is looking to roll out an Angular 2 version as that's something that is keeping us (and I'm sure many others) from moving to Angular 2. As the official Angular 2 Material team has punted until at least the end of the year until they even think about a data tables component, I think @daniel-nagy could be an excellent solution.

@Gregcop1 Gregcop1 force-pushed the feature/data-table branch 4 times, most recently from 39081b6 to c7b8774 Compare March 24, 2016 17:52
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 91.252% when pulling c7b8774 on Gregcop1:feature/data-table into 68019e8 on justindujardin:master.

@Gregcop1 Gregcop1 force-pushed the feature/data-table branch from c7b8774 to 489fbf6 Compare March 24, 2016 18:46
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 91.252% when pulling 489fbf6 on Gregcop1:feature/data-table into 68019e8 on justindujardin:master.

@Gregcop1 Gregcop1 force-pushed the feature/data-table branch from 489fbf6 to 99b34e8 Compare March 24, 2016 20:11
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 91.218% when pulling 99b34e8 on Gregcop1:feature/data-table into 68019e8 on justindujardin:master.

@Gregcop1 Gregcop1 force-pushed the feature/data-table branch from 99b34e8 to 4d46ed0 Compare March 24, 2016 20:40
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 90.793% when pulling 4d46ed0 on Gregcop1:feature/data-table into 68019e8 on justindujardin:master.

@Gregcop1 Gregcop1 force-pushed the feature/data-table branch from 4d46ed0 to 04d1152 Compare March 24, 2016 21:00
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 91.277% when pulling 04d1152 on Gregcop1:feature/data-table into 68019e8 on justindujardin:master.

@ollwenjones
Copy link
Collaborator

I think a good way forward would be go merge this PR, as an obvious first step. Anything else should be able to build atop this.

Seems like after that the options are:

  1. Continue building our own thing here.
  2. Wait for ng2 of @daniel-nagy's

I still maintain that it adds value to have a table component that encapsulates common table behaviors (like rendering a row for each model entry), and Angular 2 allows us to encapsulate more and still offer a compose-able template structure that was never possible in Angular 1. We don't need to redefine the whole table structure top-to-bottom for every unique implementation anymore. 🙌

My schedule is a moving target lately... but I plan to continue working towards option 1. Anybody can do option 2, obviously.

@Gregcop1 Gregcop1 force-pushed the feature/data-table branch from 04d1152 to bd099a4 Compare March 24, 2016 21:51
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 91.277% when pulling bd099a4 on Gregcop1:feature/data-table into 68019e8 on justindujardin:master.

@Gregcop1 Gregcop1 force-pushed the feature/data-table branch from bd099a4 to bdd27af Compare March 24, 2016 22:19
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 91.277% when pulling bdd27af on Gregcop1:feature/data-table into 68019e8 on justindujardin:master.

@Gregcop1
Copy link
Collaborator Author

To merge, Travis need to pass and I fight with him since 2 hours now. Did someone have a clue on this error: "Chrome 50.0.2661 (Windows 8 0.0.0) ERROR: DOMException{stack: 'Error: Failed to execute 'setAttribute' on 'Element': '*ngIf' is not a valid attribute name." ?

In local tests pass.

@justindujardin
Copy link
Owner

@Gregcop1 yeah, Canary doesn't properly recognize * template expansions. It fails to validate them. I believe it's an angular issue.

See how I worked around it by using [ngIf] expanded form: 603ba25#diff-91eaecea6eb363d5eba3ee6115766f00R12

@ollwenjones
Copy link
Collaborator

FYI I picked up the discussion on issue #31, since it should have been there, and not here.

refactor: selectable is now activate by an @input instead of a class
feat: selectable embed value and fire events on change
refactor: code review
feat: add some tests about selecatable data table
refactor: revamp all selectable process to follow observable pattern
fix: should repair Travis build about ngIf
@Gregcop1 Gregcop1 force-pushed the feature/data-table branch from bdd27af to 9e8f0ac Compare March 25, 2016 18:21
@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 91.135% when pulling 9e8f0ac on Gregcop1:feature/data-table into 68019e8 on justindujardin:master.

@Gregcop1
Copy link
Collaborator Author

Oh Nice. Thank you

@justindujardin justindujardin merged commit 9e8f0ac into justindujardin:master Mar 27, 2016
@justindujardin
Copy link
Owner

@Gregcop1 thanks for this contribution, and starting a great conversation about data table design. I made a few cleanups and extended your demos a bit, and will deploy it later today. 👍

It seems that there are (at least) two different ways you might want to go about building data tables in your apps. I would suggest that they are both legitimate use-cases and should be supported.

The docs could simply talk about the "two ways to build tables" and people could mix/match as fits their needs. We could have examples for building with normal table markup, and custom markup to remove boilerplate.

@daniel-nagy your data table is very impressive:exclamation: If you would like to collaborate or contribute, that would be super welcomed.

@daniel-nagy
Copy link

@justindujardin Thanks! I will be releasing an updated version of my data-table here in a bit.

I would like to port my existing module to Angular2 sometime in the near future. I would be more than happy to collaborate on that experience.

@Gregcop1 Gregcop1 deleted the feature/data-table branch March 27, 2016 20:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants