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

Issues with iteration of table rows #36

Closed
bbrink68 opened this issue Apr 13, 2016 · 3 comments
Closed

Issues with iteration of table rows #36

bbrink68 opened this issue Apr 13, 2016 · 3 comments

Comments

@bbrink68
Copy link
Contributor

Hey there, great package!

I did notice something that I'm not totally on board with. in the TableSortable.html file, you are iterating through the rows like this:

<tr *ngFor="#object of data | orderBy : convertSorting()">
    <td *ngFor="#key of object | mapToIterable; #i = index" [innerHtml]="object[columns[i].variable] | format : columns[i].filter"></td>
</tr>

So you're iterating over the rows (data) and calling the variable key on columns to get the value with object[columns[i].variable]. The reason I have issues with this is that if you pass the tablesortable component rows that include other keys that you do not plan to display in the table, you get undefined errors on columns[i].variable because you don't want to see certain row values, you didn't add them to the columns array and because you are iterating over the rows, it errors.

What I'd like to propose is iterating over the columns themselves - so that you're only ever calling for what actually has been defined to display from the rows. Something like this:

<tr *ngFor="#object of data | orderBy : convertSorting()">
    <td *ngFor="#column of columns; #i = index" [innerHtml]="object[column.variable] | format : column.filter"></td>
</tr>

This way, we don't need to handle / reduce / process the rows array any further after getting it from the backend / API, and only the items placed in columns will be shown.

What do you think?

@coryshaw1
Copy link
Member

This is much cleaner, and I definitely agree. It removes the need to map the key/value pairs of an arbitrary object using that mapToIterable pipe, since columns is already a defined, iterable type.

We won't need the index anymore like so:

<tr *ngFor="#object of data | orderBy : convertSorting()">
    <td *ngFor="#column of columns" [innerHtml]="object[column.variable] | format : column.filter"></td>
</tr>

Feel free to make a pull request if you don't want to wait. If not, this will be added to the next release.

Will close this once, this is fix is live in master.

Cheers!

@bbrink68
Copy link
Contributor Author

Sure, I'll try to get a PR done today at some point, thanks!

@coryshaw1
Copy link
Member

Closed with #37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants