Updates to the react-pivottable component

[prakhargoel-beacon]

This pull request adds in a number of pivot-table features. Primary among them are:

• Ability to toggle row and column totals.
• Subtotals (loosely based on Subtotal.js)

The subtotals renderers work with the heatmap logic and support collapsing sub-entries. They also play nicely with the aggregation framework, callbacks, etc... I've attached a screenshot below demonstrating the subtotal facility (the bottom table has rowTotals disabled).

(Apologies in advance for dumping such a large PR on your lap. I'd be happy to explain the changes and to update it to conform to your coding guidelines, etc...)

Thanks.
-- PG

[nicolaskruchten]

Interesting, thank you!

Perhaps instead of modifying TableRenderers it might be best to leave that file alone and create a new component SubtotalTableRenderers beside it, and then surgically see what can be pulled out as common components? as it stands right now the diff is hard to read :)

[nicolaskruchten]

are the changes here just refactoring or are they required for the new renderer to function?

[nicolaskruchten]

The same might actually apply to PivotData ... instead of parameterizing it, perhaps it would make sense to have a subtotal-aware copy of it? that's less clear, and is one of the reasons why Subtotal.js is apart from the main PivotTable.js library.

[prakhargoel-beacon]

Nicolas,

Totally understood. This is a pretty large PR. And apologies for taking so long to get back to you. Work just kept getting in the way.

For better understanding the changes:

• Commit dbc8168 is fairly minor and adds in the rowTotal and colTotal options.
• Commits f067c1d through f097b1c refactor the table renderers to make them a little easier to understand and modify. They don't change the functionality and mostly just move code from the primary render method to separate methods within the same class.
• Commit 391be64 is where the bulk of the subtotal changes are. Hopefully they should be much easier to understand vs. commit f097b1c since I kept the same basic code layout.
• Commit c9a2067 refactors the PivotTableUI just a little bit to make it easier to customize (I use this outside the react-pivottable package). This commit is pretty independent from the rest.
• The rest of the changes are minor cleanups, updating for babel presets that I hadn't realized had been a problem, etc...

Regarding splitting out the subtotal code:

• I thought about that when writing this. I ultimately settled on keeping them in the same class because there is quite a bit of shared code between the two.
• In PivotData:
  • The only meaningful change is in processRecord where we loop through all prefixes of the row and column keys instead of just the entire thing.
  • There are a handful of minor changes to make shorter key arrays sort either before or after the longer arrays (based on user settings).
• In TableRenderers:
  • the span calculation needs to be slightly modified to account for shorter keys.
  • I pre-compute the callbacks because that speeds things up a little bit (but it's not really necessary).
  • I had to add in a little more argument parsing.
  • The biggest changes are for the expand/collapse functionality.
    • I added in a filtering step to remove collapsed (and therefore hidden) keys.
    • I needed a little bit of extra logic to add in spacing cells for keys corresponding to subtotals.
    • And I had to add in some logic to populate the arrow characters with the associated click handlers.
  • BUT: the underlying code structure and methods used are still pretty much identical to the non-subtotal case.

Hope this helps!

[nicolaskruchten]

OK, thanks for the detailed response! I promise to take a close look in the coming week: I'm just buckling down for a big release at the moment.

[nicolaskruchten]

One thing which would make both reviewing and merging easier is the addition of a test or two for the modifications to PivotData ;)

[prakhargoel-beacon]

That I can do.

Also if it'll help, I can re-arrange the commits into something a bit more logical. Feel free to ask.

[nicolaskruchten]

Your description of the commits is great, no need to rearrange anything! A test would help and then it's a question of me finding the time to QA/review :)

[prakhargoel-beacon]

FYI: I added in that test for PivotData and the prepare script. Just wondering if you had any other requests.

Thanks.

[newt0311]

Hi,

Just wondering if you'd gotten a chance to take a look at this.

Thanks.

[newt0311]

Hello again Nicolas,

Got a chance yet? Any way I can help?

Thanks.

[nicolaskruchten]

I'm looking at this today. Apologies on the delay.

[nicolaskruchten]

OK so I cloned this and ran yarn update && yarn start to run the demo app, but the Table renderer throws React errors. Are you seeing this also? Can you fix this please?

[nicolaskruchten]

In terms of getting this merged in, this really is too big a PR to easily review... Do you think you could submit a series of smaller ones or something? Maybe one for the row/total columns? And a separate one that just does some of the cleanup/refactoring without changing the behaviour? all in the service of making the bigger behaviour-change PR easier to review?

Sorry, I know it's a big ask after a long silence, but I would like to get this sorted and release your hard work!

[newt0311]

I'll take a look at the errors and split up the pr. Thanks for taking a look.

[nicolaskruchten]

Did you still intend to look at the errors and/or break up this PR into more-digestible chunks?

[newt0311]

Yes. Eventually... Work has just gotten busy.

(And yes, I recognize the irony in pushing for a review and then holding up progress myself. My sincere apologies.)

[nicolaskruchten]

No worries ;)

[mikkelking]

Any progress on this - I'm presuming the PR is a step forward for the module... is it best to spend more time and break it up into pieces, or accept it as is?
I would like to add a module for Vega-lite renderers. It doesn't look too hard to do. My quandary is which branch to use to base my work on. I am happy to help if needed

[newt0311]

I recommend adding to the current master. I don't know when I'll get around to updating this PR and besides the changes are pretty self contained in the table renderers.

[thetwosents]

Bump. Willing to sponsor project updates.

[nicolaskruchten]

@thetwosents please email me at nicolas@plotly.com and we can chat about sponsorship?