-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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] Print export #2519
[DataGrid] Print export #2519
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.
I had a quick look and it's working fantastically. 👌
- Did you think about exposing the function below as a
disableVirtualization
API call? It could be used instead of updating the state.
-
We could add a few CSS rules with
@media print
to make the grid print-safe. The left and right borders could be removed, the same for the padding. -
In the full-featured demo, when I close the print window it shows columns that were hidden before, like the ID.
@m4theushw thanks for the review. I'll wait to see what the others have to say and fix the issues. For the I was thinking of adding the CSS rules. We can add a few but that's why I added the option for people to provide their own riles directly. I'll check about the hidden columns, that shouldn't happen |
Concerning the rename of the CSV option interface We will need to make a PR in the core (adding also the print at the same time I suppose) and then delete this file (and the |
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.
It looks like a great start. We are going further than what I was expecting for the first iteration, which is cool. I would propose we offer two different levels of solution:
- You have built an API to turn the rendering of the grid in a print-ready mode. I would propose that we double down on it:
- There is a rendering bug on http://0.0.0.0:3001/components/data-grid/export/#csv-export, only one row is visible in the print version
- There is a rendering bug on http://0.0.0.0:3001/components/data-grid/export/#csv-export, the restored state is not identical to the initial one. Related to [DataGrid] Print export #2519 (comment)
- We could expose this as a public API. It would make it easy for developers that are used to the AG Grid API. It would allow more customizations for the developers, say they want to print chart at the same time or do some other transformations before calling
print
(Pro only?). [DataGrid] Print export #2519 (comment) - There is likely something to do about > 1,000 rows export. It prints on 20 pages, it feels that beyond, it's pointless. Same feedback for the
autoHeight
prop.
- The new export menu option is cool. It's what I mean by "going further than what I was expecting". I didn't notice something specific about it, outside of a few changes, some breaking, on the existing CSV feature that maybe should be done standalone (not sure, I assume we fixed stuff not directly related to the print).
A side thought, for the docs, we could try to sell developers to use the Print export when they need the PDF export 😁
// Revert columns to their original state | ||
apiRef!.current.updateColumns( | ||
columns.map((column) => { | ||
if (previousHiddenColumns.current.includes(column.field)) { |
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.
Instead of keeping track of a previousHiddenColumns
state, could it be simpler to store the whole column state and restore it?
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.
Unfortunately, it won't work unless I deep clone it.
setGridState((state) => { | ||
previousGridState.current = state; | ||
return { | ||
...state, | ||
containerSizes: { | ||
...state.containerSizes!, | ||
renderingZone: { | ||
...state.containerSizes!.renderingZone, | ||
height: visibleSortedRows.size * props.rowHeight!, | ||
}, | ||
renderingZonePageSize: visibleSortedRows.size, | ||
}, | ||
viewportSizes: { | ||
...state.viewportSizes, | ||
height: visibleSortedRows.size * props.rowHeight!, | ||
}, | ||
}; | ||
}); |
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.
This logic looks brittle (away from the related logic, and might duplicate). Is there a way we could call an imperative API, e.g. an enableAutoHeight()
method to move it closer to the hooks that manage containerSizes
and viewportSizes
as well as not duplicating the logic that handles the autoHeight
prop?
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.
Ok, I took a bit more time to check this more in-depth. The problem is that this logic doesn't really do enable autoHeight
neither it disables the virtualization. What it does is force the grid to render all rows (have all the HTML for the rows be in the dom but still keep the scroll so that the content on the page doesn't jump).
For example @m4theushw suggestion to reuse his logic from the useGridNoVirtualization
won't work because as far as can see there are other places where that prop is directly red and used (same as the autoHeight
, which was my original idea). I'll see if moving that chunk into useGridNoVirtualization
will make sense and expose it as an API from there.
Update: Another solution is to explain in the docs that in order to use the feature one needs to set disableVirtualization={true}
. I would love some feedback on this approach as it will only be applicable for the DataGridPro
packages/grid/_modules_/grid/hooks/features/export/useGridPrintExport.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/hooks/features/export/useGridPrintExport.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/hooks/features/export/useGridPrintExport.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/hooks/features/export/useGridPrintExport.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/hooks/features/export/useGridPrintExport.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/hooks/features/export/useGridPrintExport.tsx
Show resolved
Hide resolved
packages/grid/_modules_/grid/hooks/features/export/useGridPrintExport.tsx
Outdated
Show resolved
Hide resolved
@DanailH https://github.com/mui-org/material-ui-x/blob/next/docs/src/pages/components/data-grid/getting-started/getting-started.md We can update the |
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.
If the dark mode is used it will print with the styles from the dark mode. Since we only have the dark styles to copy I don't see an easy alternative. I think only with option 3 from #2519 (comment) we can solve that, by creating another theme to wrap the grid before rendering.
packages/grid/_modules_/grid/hooks/features/export/useGridPrintExport.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/hooks/features/export/useGridPrintExport.tsx
Outdated
Show resolved
Hide resolved
const buildPrintWindow = React.useCallback((title?: string): HTMLIFrameElement => { | ||
const iframeEl = document.createElement('iframe'); | ||
|
||
iframeEl.id = 'grid-print-window'; |
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.
Will it work without the id? If not, useId()
could be used to generate a unique value.
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.
The id
is only used for the tests. If we are going with your approach, setting the id
is not needed.
previousGridState.current = gridState; | ||
|
||
if (props.pagination) { | ||
apiRef.current.setPageSize(visibleRowCount); |
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.
This won't work if the user is controlling the page size. Forcing the state also doesn't work, because it will require a forceUpdate()
to propagate the changes, and when this runs, all effects will also run again and reset the value back to the prop value. I see some options:
- Add a escape hatch to temporarily disable the state update constraint. https://github.com/mui-org/material-ui-x/blob/95fed89847c5f1489ddb63dafedcf0d907d46354/packages/grid/_modules_/grid/hooks/features/core/useGridControlState.ts#L40
- Add a note to the docs mentioning that the print export doesn't work if
pageSize
orpage
is present. - Render the grid inside the iframe and disable the virtualization:
ReactDOM.render(<DataGrid {...props} disableVirtualization />, iframeEl)
There's going to be a dependency cycle with 3., which I believe we could solve by using require()
instead of import
.
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 would go with option 2. -> the reason is that 3. for me will be too expensive in terms of the time it will take to render.
Also, this feature (along with a couple of other ones) doesn't work with server-side pagination which is controlled. If a developer is controlling the pagination the print export has an API that they can call manually.
This looks to me like a corner case - we can see what the feedback will be and if it is problem then we can spend time fixing it.
Co-authored-by: Matheus Wichman <[email protected]>
…tExport.tsx Co-authored-by: Matheus Wichman <[email protected]>
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
* Print the grid's data. | ||
* @param {GridPrintExportOptions} options The options to apply on the export. | ||
*/ | ||
exportDataAsPrint: (options?: GridPrintExportOptions) => void; |
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 think we should rename this one UNSTABLE_exportDataAsPrint
for a few months, the time to stabilize the API and behavior.
We should probably do it for most of the new API methods.
On the Tree Data I also hide them from the documentation (@ignore
) but the UNSTABLE_
may be enough
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'm with this as long as we keep it as a general rule of thumb - new API methods should be marked as unstable @m2mathew @oliviertassinari what do you think?
Update: I'll keep this one as it is because I use it on the toolbar button directly. But I marked the disableVirtualization
one as unstable.
@@ -58,7 +58,8 @@ dataGridComponentAPI.children = [ | |||
{ pathname: '/api-docs/data-grid/grid-col-def', title: 'GridColDef' }, | |||
{ pathname: '/api-docs/data-grid/grid-cell-params', title: 'GridCellParams' }, | |||
{ pathname: '/api-docs/data-grid/grid-row-params', title: 'GridRowParams' }, | |||
{ pathname: '/api-docs/data-grid/grid-export-csv-options', title: 'GridExportCSVOptions' }, |
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.
How do we avoid having 404 on the doc while we update the core page list ?
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 can merge that and then open a PR against the core with this change.
packages/grid/_modules_/grid/models/api/gridDisableVirtualizationApi.ts
Outdated
Show resolved
Hide resolved
@m2mathew @flaviendelangle I would like to proceed with merging this and releasing it to see what the feedback will be. Ideally, I would like to merge it tomorrow or today so that it is included in this week's release. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
As we discussed I'm merging this. |
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.
The print button is not working. We should fix this before the release.
Fixes #200
This is the initial version of the Print Export feature. I'll keep the PR as a Draft for now as tests and docs are missing from the PR and I would like to discuss the approach and features before adding the tests and docs and converting it to normal PR.
Preview: https://deploy-preview-2519--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-toolbar--export
Explanation:
In the
Export
drop-down menu in theGridToolbar
is now an additional button calledPrint
. The remaining API is part of theuseGridPrintExport
hook. The setup is identical to the CSV Export feature.Options available for the Print Export:
These options can also be passed to the
GridToolbarExport
component (same as the CSV export).ToDo: