Skip to content

Conversation

@LorenMaxwell
Copy link
Contributor

Allan, my first ever pull request for anything -- so my apologies if I don't get it quite right!

At any rate, this change is based on the very last part of this discussion: https://datatables.net/forums/discussion/comment/123641/#Comment_123641

My suggestion was to have an option to hide the row group if all the rows are the same.

I've added an option called "hideIfSame" with a default of false.

Although Github's comparison shows multiple lines changed under the _draw function, the only change was to wrap the loop in an if statement.

Please let me know if you have any questions or if there's anything I can do to help make this better (i.e., convince you to add this feature!).

If you incorporate this, it's available under the same MIT license as the DataTables project.

@LorenMaxwell
Copy link
Contributor Author

LorenMaxwell commented Jan 30, 2018

Allan, I don't quite have something right. I tested this with a pageLen of -1, which shows all rows, and it worked fine.

However when testing it with pagination, it doesn't work properly.

I'll close this pull request and try again after troubleshooting.

In the meantime I welcome any thoughts on where I might have missed it.

@LorenMaxwell
Copy link
Contributor Author

I approached this from a different angle that fixed my previous issue with pagination.

Now the constructor checks if there is more than one group and if the hideIfSame option is false. If either of those conditions exists, then it executes as normal, otherwise the other functions are simply skipped altogether.

On a separate issue, I noticed that "No group" (or whatever the emptyDataLabel is set to) was not being treated as an actual group.

When a table is rendered, each and every record that is not part of a group had "No group" inserted above it, even if it was part of a continuous group of rows with "No group".

Seems like rows falling under the "No group" group itself should be grouped like any other group under a row group . . . :-)

Anyway, maybe that was an intentional design decision, but I included a change that treats the "No group" as an actual group.

@LorenMaxwell LorenMaxwell reopened this Jan 30, 2018
@LorenMaxwell
Copy link
Contributor Author

Made it a little more versatile by giving an option to hide the row grouping if all the rows belong to unique groups (i.e., rowGroup: "name" on the example data with "Tiger Nixon", etc.).

Now the two proposed new options are hideIfAllSame and hideIfAllUnique.

@DataTables
Copy link
Collaborator

Absolutely fantastic - thank you! I'm not going to pull it in right now, because if I do it will close and I'll forget to add the documentation and examples for it, so I'll leave it open atm, and hopefully pull it in tomorrow. I think this is a nice addition - thanks!

@LorenMaxwell
Copy link
Contributor Author

Thanks, Allan. Very glad to contribute back and happy to see how easy it was through Github.

Once you get it pulled, do you know when you might release v1.0.3 on the DataTables site?

@DataTables DataTables merged commit 6f6bd25 into DataTables:master Feb 19, 2018
@DataTables
Copy link
Collaborator

In this example: http://live.datatables.net/ripikumo/1/edit - should the rows grouping not be hidden?

@LorenMaxwell
Copy link
Contributor Author

It should show the grouping, right? Not all ages are the same but they're not all unique either.

@DataTables
Copy link
Collaborator

Hmmm. The grouping is unique, but yes you are right that the ages are grouped. But the grouping is still showing as unique.

How about this one: http://live.datatables.net/ripikumo/2/edit . The table is filtered to that the rows are unique, but the grouping is still shown.

@LorenMaxwell
Copy link
Contributor Author

Well, that's the behavior I intended since it's based on the whole dataset, but I see how you're understanding it.

Perhaps in hindsight "hide" isn't as good of a descriptor as "disable"?

And perhaps it should actually disable the row grouping through API instead of exiting the extension from the constructor before it does anything more than compare the numer of groups to the number of rows.

Alternatively the extension could be updated with the behavior you're expecting.

@DataTables
Copy link
Collaborator

DataTables commented Feb 22, 2018

Been thinking about this over the last few days and I think for the moment I'm going to unmerge the change I'm afraid. What I'd like to see to make this feature complete is support for:

  1. Hiding the grouping based on the entire data set (unfiltered), which is what you had (although it didn't take into account any updates in the data)
  2. Hiding the grouping based on the filtered data set
  3. Hiding the grouping based on the current page only

For example hideIfUnique could be an array: [ 'all', 'filtered', 'page' ] for all of the above and the developer using them can selectively enable or disable the options they want.

I think its a very useful feature and I will add that in future, but I suspect the current implementation will lead to confusion similar to my own.

Edit: Created #7 for this

@DataTables DataTables mentioned this pull request Feb 22, 2018
@LorenMaxwell
Copy link
Contributor Author

LorenMaxwell commented Feb 23, 2018

Yeah, I can see how more option would be useful.

I hadn't thought about it in the way you had.

Meanwhile I use this bit of code to disable the row group when they're all the same:

.on("init", function() {
    var groups = table.column("[insert column name here]:name").data().unique();
    (groups.length == 1?table.rowGroup().disable().draw():table.rowGroup().enable().draw());
})

I'll post this in the DataTables forum in case anyone else would like to use it.

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

Successfully merging this pull request may close these issues.

2 participants