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

Feature/complex headers #751

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

mtyszler
Copy link

@mtyszler mtyszler commented Jan 6, 2020

Fixes #418.

This version builds on original suggestions from @chrisvwn.

It creates a function that reads multi-row headers into an array of arrays, where each array row is a header row.

Further it updates csv, copy, pdf, excel and print to process the multi-row header object.

For excel it merges the cells as shown on screen.

It supports colVis

It does not update the flash counter parts of html5 functions

(cherry picked from commit 3178277)
…g with minor adjustments changes from https://github.com/chrisvwn/DT/blob/master/inst/htmlwidgets/lib/datatables-extensions/Buttons/js/dataTables.buttons.min.js, which is inpired by  https://datatables.net/forums/discussion/comment/106434/#Comment_106434

This prepares the dataTables.buttons.min.js to handle multiple row headers. Without further adjustments this change actually create problems for all output functions, which expect a single row header instead of an array.
… and copyHtml5, allowing it to read multiple row headers
…justments to what is suggested.

This alloes the output function 'Print' to handle multiple row headers
…n commit ed15f2f allowing the outputfunction pdfHtml5 to print pdf with multi-row headers
…n commit ed15f2f improves the outputfunction excelHtml5 to export excel files with multi-row headers, without the merges. On top of what is described  rstudio#418, it makes other modifications needed to match the latest rstudio/DT version.
…n commit 6b56d58 and process the col/row span information to merge the cells at the excel output  via the function excelHtml5
…roduced on ed15f2f. Here the main improvements are:

* It includes all headers, including the ones which are hidden using colvis, for example. This if done via using aoHeader instead of nThead, and then using the cellIndex property instead of the colspan of the shown table on screen.
* After extracting a complete rawHeaderMatrix, it process it to remove hidden columns and format the content. (similar to the original single row header code)
Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

@mtyszler Many thanks!

@shrektan My major concern is whether we'll be able to maintain these scripts in the future, because all JS files are automatically downloaded from DataTables' website. If we run the automated script again in the future, these files may be overridden.

@shrektan
Copy link
Collaborator

shrektan commented Jan 21, 2020

@yihui I share the same concern, too. These are not simple hacks. Even if we find out a way to upgrade the js files with this patch maintained (one possible way is to apply a git patch), I can't guarantee it will always work in the future because the upstream source may contain big changes.

I incline that this PR is more suitable for the upstream js library but you have more experience on the topic of maintainance.

@yihui
Copy link
Member

yihui commented Jan 21, 2020

Absolutely. If this could be done in the upstream library instead, we can easily pick it up from there in the future.

@mtyszler
Copy link
Author

Thank you @yihui and @shrektan for your review and comments. I wasn't aware that the code was auto-imported via script and I understand your concerns.

In the upstream library there is an open PR (DataTables/Buttons#55) with similar topic. However, it has been open over 5 years ago with last comment (DataTables/Buttons#55 (comment)) from @DataTables is a bit ambiguous in how they want to proceed.

I'm not sure I have the time now to reproduce my solution at a fork of https://github.com/DataTables/Buttons, but if I do I might try.

I'll ask @DataTables if they would be interested in that before proceeding.

So I take you'd prefer not to merge this PR, right?

@yihui
Copy link
Member

yihui commented Jan 22, 2020

That's right.

The worst case is that if users really want this, we provide an R script or function to download the patched version of these JS files and replace the ones in DT. Of course, we should also warn them that the next time they update DT, they should run the R script or function again.

@mtyszler
Copy link
Author

Dear @yihui and @shrektan.

I just submitted DataTables/Buttons#170 to the upstream library, as you suggested.

I suggest you keep this PR open and close it when/if upstream changes are taken up and picked-up by you.

Best regards,

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck 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.

Multi-row headers + rowspan/colspan in xlsx headers
4 participants