Add support for row grouping and collapsed state in BeginRow #63
Conversation
|
Thanks for your contribution @mutludev , I'm trying having a look into it during this weekend! |
salvois
left a comment
salvois
left a comment
There was a problem hiding this comment.
Hi @mutludev ,
thank you very much for your contribution: it looks quite good to me.
Would you mind adding some explanation on how the new API is meant to be used, and unit tests for the new functionality?
I also added comments in a couple of spots just for clarification.
Thanks,
Salvo
| } | ||
|
|
||
| public XlsxWriter BeginRow(double? height = null, bool hidden = false, XlsxStyle style = null) | ||
| public XlsxWriter BeginRow(double? height = null, bool hidden = false, XlsxStyle style = null, bool collapsed = false, int? groupLevel = null) |
There was a problem hiding this comment.
Any particular reason to name the parameter groupLevel rather than outlineLevel term used in the specification?
There was a problem hiding this comment.
You're right; outlineLevel will be clearer. I'll modify it.
| _streamWriter.Write("\""); | ||
| _needsRef = false; | ||
| } | ||
| if (collapsed) |
There was a problem hiding this comment.
If I understand the specification correctly, there is some correlation between the collapsed flag and the hidden flag of rows. Is this correct? There could be illegal combinations allowed by the API?
There was a problem hiding this comment.
Yes, I looked into it again. If the collapsed flag is true, then all rows in the group should have their hidden flag set to true as well. Toggling a group essentially toggles both the collapsed and hidden values. I'm not sure how we should handle this; one option might be to track when collapsed is set to true, and then automatically set the hidden property of the following rows to true. But I'm not sure if that would be overkill.
| StyledLarge.Run(); | ||
| StyledLargeCreateStyles.Run(); | ||
| Zip64Huge.Run(); | ||
| Grouping.Run(); |
There was a problem hiding this comment.
I'd rather move this before the heavier examples (InlineStrings being the first) so that you can interrupt the Examples program if you just need quick examples.
| xlsxWriter | ||
| .BeginWorksheet("Sheet 1") | ||
| .BeginRow().Write("Header 1").Write("Header 2").Write("Header 3") | ||
| .BeginRow(groupLevel: 1).Write("Test 1").Write("Test 2").Write("Test 3") |
There was a problem hiding this comment.
Maybe some more combinations of outline levels and collapsed levels could be helpful to better showcase the API.
|
Hi @salvois, Thanks for the clarification. I'll try to address your comments this weekend if I can. Also, I left a comment about the correlation between collapsed and hidden; could you take a look when you have time? Thanks, |
## Continues https://github.com/mutludev salvois#63 ## Changes - copied the code from salvois#63 - adjusted naming to `outlineLevel` - added tests ## Disclaimer This is just a continuation of the work done by mutludev due to inactivity on original PR
2 participants
This update adds support for Excel row grouping by introducing two new optional parameters to BeginRow:
collapsed— whether the row group should be collapsed by defaultgroupLevel— the outline level for groupingI'm not completely sure about adding collapsed and groupLevel directly to BeginRow. It works, but it feels a bit awkward to keep extending this method with more parameters. Open to suggestions if there's a better place or pattern for this.