-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add pivot table with limitations #2551
Conversation
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) { | ||
throw new Error('No pivot table rows specified.'); | ||
} | ||
|
||
if (!model.columns.length) { | ||
throw new Error('No pivot table columns specified.'); | ||
} | ||
|
||
if (model.values.length !== 1) { | ||
throw new Error('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 } | ||
// ] | ||
|
||
const names = 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 making wb.workbook.pivotTables
a getter returning pivottables from all worksheets? Or .. a function?
Can it attach one pivotTable
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 of worksheet.pivotTables
and worksheet.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 mutate worksheet.pivotTables
or worksheet.workbook.pivotTables
and only access worksheet.addPivotTable
.
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. |
```js worksheet.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 discussion here. |
Note: Pivot table support is in its early stages with certain limitations, including:
Example usage