- Notifications
You must be signed in to change notification settings - Fork1.9k
Add pivot table with limitations#2551
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hello@mikez , Thank you for the latest changes, I'm really happy that Exceljs has possibility to gain new feature even with limitations.
I added few comments, I would be glad for answering that 😄
Good job and I love the direction when it comes. 🥇
| if(!model.rows.length){ | ||
| thrownewError('No pivot table rows specified.'); | ||
| } | ||
| if(!model.columns.length){ | ||
| thrownewError('No pivot table columns specified.'); | ||
| } | ||
| if(model.values.length!==1){ | ||
| thrownewError('Exactly 1 value needs to be specified at this time.'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would It be enabled to add partially configured pivot table without for instance rows set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
At this time, the code doesn't support that. You need at least one item in each of {rows, columns, values}.
However, this could be a potential feature down the road if it is requested.
| // { name: 'E', sharedItems: null } | ||
| // ] | ||
| constnames=worksheet.getRow(1).values; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does it always start from row 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I made the simplifying assumption that the source data is represented by the entire sheet, with the first row representing the header. Do you think this assumption is too simplistic and won't capture 95% of the cases?
| this.pivotTables.push(pivotTable); | ||
| this.workbook.pivotTables.push(pivotTable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What will happen if someone remove the pivot table from one of these places, or overwrite one of them?
What do you think about makingwb.workbook.pivotTables a getter returning pivottables from all worksheets? Or .. a function?
Can it attach onepivotTable to multiple sheets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What will happen if someone remove the pivot table from one of these places, or overwrite one of them?
@Siemienik It seems you're pointing at the mutability ofworksheet.pivotTables andworksheet.workbook.pivotTables and concerns for that clients might mutate this. If you think this type of safety is needed and consistent with elsewhere in ExcelJS, we can definitely make it a getter. I'll take your advice here.
Can it attach one pivotTable to multiple sheets?
At this time, I intend for there to be at most one pivotTable in at most one sheet across the entire workbook. The user should not have to mutateworksheet.pivotTables orworksheet.workbook.pivotTables and only accessworksheet.addPivotTable.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| return` | ||
| <pivotField axis="${axis}"${defaultAttributes}> | ||
| <items count="${sharedItems.length+1}"> | ||
| ${[...range(0,sharedItems.length)].map(index=>`<item x="${index}" />`).join('\n ')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You may try this way:
| ${[...range(0,sharedItems.length)].map(index=>`<item x="${index}" />`).join('\n ')} | |
| ${sharedItems.map((item,index)=>`<item x="${index}" />`).join('\n ')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Clever. I like this. I will update and force push this.
Thank you. Please guide me where I might want to add clarifications to the documentation or comments to make it clearer for other readers. |
```jsworksheet.addPivotTable(configuration);```**Note:** Pivot table support is in its early stages with certain limitations, including:- Xlsx files with existing pivot tables can't be read (writing is supported).- Pivot table configurations must have one "value"-item and use the "sum" metric.- Only one pivot table can be added for the entire document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, thank you for booting up new feature into exceljs 👏
👉 Feedback and discussionhere. |
Uh oh!
There was an error while loading.Please reload this page.
Note: Pivot table support is in its early stages with certain limitations, including:
Example usage