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.

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

[DataGrid] Add support for getRowsToExport option to print export #10084

Merged

Conversation

zreecespieces
Copy link
Contributor

@zreecespieces zreecespieces commented Aug 18, 2023

Fixes #9993
Fixes #9824

I created this PR in response to an issue I submitted regarding the Grid Print Export feature.

There was an inconsistency between the behavior and implementation of the CSV export and Print Export. When rows are selected and CSV export is chosen then only the selected rows are exported. However, when attempting to export selected rows using the Print option then all of the rows were being exported and the selection was not being honored.

This PR fixes this issue and attempts to make the implementations more similar and consistent.

@mui-bot
Copy link

mui-bot commented Aug 18, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-10084--material-ui-x.netlify.app/

Updated pages

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms -201.1 -76 -201.1 -135.8 48.552
Sort 100k rows ms 686.6 1,296.8 1,296.2 1,121.06 225.417
Select 100k rows ms 601.9 732.9 609.8 652.16 56.516
Deselect 100k rows ms 123.9 326.3 178.2 196.66 73.143

Generated by 🚫 dangerJS against 3767d7a

Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

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

Thank you for submitting the solution, the solution looks clean and solid to me 🎉

One question that comes in my mind is, should we persist the checkboxes in the print version (like we did before this change), at least when there is no selection (possibly by skipping calling the updateGridRowsForPrint when there's no selection.) Will that be a user's expectation in some use-cases?

P.S You might need to run yarn docs:api to regenerate the docs and fix the build.

@MBilalShafi MBilalShafi added component: data grid This is the name of the generic UI component, not the React module! regression A bug, but worse feature: Export labels Aug 20, 2023
@MBilalShafi MBilalShafi changed the title implemented fix for export as print row selection [DataGrid] Fix print export to honor the selected rows during export Aug 20, 2023
@cherniavskii cherniavskii self-requested a review August 29, 2023 14:36
@zreecespieces
Copy link
Contributor Author

zreecespieces commented Sep 4, 2023

Sorry for such a late reply @MBilalShafi - I've been swamped at work and traveling so just haven't had much free time until now.

That's a good point to discuss. However, it seems that the current behavior regarding preserving checkboxes already does not include the checkboxes on print export. As of the time of my writing it seems that regardless of whether or not rows are selected the checkboxes are NOT preserved in the print export. I think that this behavior actually more closely matches user expectations for the following reasons.

First, the checkboxes are already not preserved when exporting as CSV and so omitting them from the print export would maintain consistency between the two methods. Second, generally users export data for reporting, presentation, or analysis. Visual elements like checkboxes don't carry data significance and thus are not necessary in the exported content. IMO the checkboxes clutter the data representation unnecessarily as the user is anticipating getting the data, not the UI elements. Third, in a digital interface a checkbox provides an interactive element for selection. In a printed or exported static format its functional relevance is lost. These all lead me to believe that omitting the checkboxes for print export would deliver the best user experience.

As I mentioned it seems that is now the current behavior, anyways. Here's a sandbox using the latest package version (without my code changes). Notice that when exporting via print the checkboxes are never included.

I do think that only calling updateGridRowsForPrint if there are selected rows makes sense so I have implemented that change.

Although not the typical use case I do believe providing an option to preserve the checkboxes, such as includeCheckboxes, that defaults to false, would be the best solution in terms of flexibility and customization for the end user. As such I have implemented that functionality as well. Let me know what you think.

I also fixed an issue in my initial implementation where the Grid footer was not being correctly placed when exporting selected rows via print. Instead of appearing directly underneath the selected rows it appeared at the bottom of the page where it would have been had no rows been selected. This is now fixed and appearing as expected in the correct place.

I have ran the appropriate commands to ensure code formatting and new docs generation as well as syncing up with the master branch so hopefully this PR is ready for review. If there's any other issues or points of discussion I'd be happy to address them.

@cherniavskii cherniavskii changed the title [DataGrid] Fix print export to honor the selected rows during export [DataGrid] Add support for getRowsToExport option to print export Sep 20, 2023
@cherniavskii cherniavskii added enhancement This is not a bug, nor a new feature and removed regression A bug, but worse labels Sep 20, 2023
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Thank you!

@cherniavskii
Copy link
Member

I've opened #10410 to change the default behavior in v7, as we discussed 👍🏻

@cherniavskii cherniavskii merged commit 82d44ea into mui:master Sep 20, 2023
@zreecespieces
Copy link
Contributor Author

amazing. Happy to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature feature: Export
Projects
None yet
6 participants