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.

Sign up for GitHub

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

Jump to bottom

[DataGridPremium] Add excel export #3802

Closed
wants to merge 21 commits into from
Closed

[DataGridPremium] Add excel export #3802

wants to merge 21 commits into from

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Jan 31, 2022 •
edited
Loading

Depends on #3806

TODO

  • export pipeline
  • reflect column types in excel
  • reflect tree structure

preview

@mui-bot
Copy link

mui-bot commented Jan 31, 2022 •
edited
Loading

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 198.9 383.8 295.5 293.02 78.067
Sort 100k rows ms 327.3 792.1 582.7 577.56 153.087
Select 100k rows ms 149.9 364.7 196.5 216.92 77.231
Deselect 100k rows ms 96.1 183.5 183.5 141.42 36.904

Generated by 🚫 dangerJS against f12d3ef

@alexfauquette alexfauquette self-assigned this Jan 31, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 31, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@alexfauquette alexfauquette requested a review from DanailH February 1, 2022 09:16
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 1, 2022
@github-actions
Copy link

github-actions bot commented Feb 1, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 1, 2022
DanailH
DanailH reviewed Feb 2, 2022
View reviewed changes
Copy link
Member

@DanailH DanailH left a comment

Choose a reason for hiding this comment

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

Looks like a great starting point!

package.json
@@ -181,6 +181,7 @@
]
},
"dependencies": {
"exceljs": "^4.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense for direct dependencies to be locked with ~?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have an opinion on this subject. I had a look at core repo, all the dependencies are with ^

packages/grid/_modules_/grid/components/toolbar/GridToolbarExport.tsx Show resolved Hide resolved
packages/grid/_modules_/grid/hooks/features/export/serializers/excelSerializer.ts

const excelJS = await getExcelJs();
const workbook: Excel.Workbook = new excelJS.Workbook();
const worksheet = workbook.addWorksheet('Sheet1');
Copy link
Member

Choose a reason for hiding this comment

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

Should we allow the name of the sheet to be passed as an input?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it, but as long as we use only one sheet per workbook, I don't see the interest

packages/grid/_modules_/grid/hooks/features/export/serializers/excelSerializer.ts Outdated
const { columns, rowIds, getCellParams, includeHeaders } = options;

const columnsWithoutCheckbox = columns.filter(
(column) => column.field !== GRID_CHECKBOX_SELECTION_COL_DEF.field,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can remove the checkbox column when the columns are being organized, in the useExcelExport hook?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it makes sense. I also moved this filter from csvSerializer to useCSVExport to have a utils used by both the CSV and the Excel

packages/grid/_modules_/grid/hooks/features/export/serializers/excelSerializer.ts Show resolved Hide resolved
packages/grid/_modules_/grid/hooks/features/export/serializers/excelSerializer.ts Show resolved Hide resolved
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 3, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 4, 2022
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@alexfauquette alexfauquette mentioned this pull request Feb 8, 2022
Merged
@siriwatknp
Copy link
Member

siriwatknp commented Feb 8, 2022 •
edited
Loading

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

  1. pull latest master from upstream to your branch
  2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
    2.1 revert the change on those markdown files you did
    2.2 pull latest master from upstream (you should not see conflict)
    2.3 make the changes again at docs/data/data-grid/*
  3. run yarn docs:api
    • you might see the changes in docs/pages/x/api/data-grid/* if your PR touches some of the components
    • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

@alexfauquette
Copy link
Member Author

The modification of the file structure implies too much conflicts. I started another branch #3981

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m4theushw m4theushw m4theushw left review comments

@DanailH DanailH DanailH left review comments

Assignees

@alexfauquette alexfauquette

Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alexfauquette @mui-bot @siriwatknp @DanailH @m4theushw