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

[data grid] Improve print support #6273

Merged
merged 1 commit into from
Sep 27, 2022
Merged

[data grid] Improve print support #6273

merged 1 commit into from
Sep 27, 2022

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Sep 23, 2022 •
edited
Loading

This seems to be an opportunity to:

  1. Fix [data grid] Print export scrolls the page to the bottom #6272
  2. Show the print window faster, no need to wait for the page to load
  3. Remove the HTTP header iframe limitation
  4. Remove the dependency on [core] Upgrade @mui/monorepo #4149 (comment)

Before: https://mui.com/x/react-data-grid/export/#default-toolbar
After: https://deploy-preview-6273--material-ui-x.netlify.app/x/react-data-grid/export/#default-toolbar

Tested in Firefox, Safari, Chrome, latest versions

I have left a couple of TODO for a clean-up follow-up. I was also surprised with how we document the print export. If you:

  1. open https://mui.com/.
  2. search for print export:

Screenshot 2022-09-24 at 00 55 30

  1. click on the first link, you will land on https://mui.com/x/react-data-grid/export/#print-export, without a demo. I was disoriented by this, I struggled a bit to find a demo where I could see the feature in action. Would it make sense to add one?

@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! feature: Export bug 🐛 Something doesn't work performance test labels Sep 23, 2022
@mui-bot
Copy link

mui-bot commented Sep 23, 2022 •
edited
Loading

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 502.7 852.8 602.8 660.78 152.03
Sort 100k rows ms 573.4 1,098.5 665.9 813.3 188.072
Select 100k rows ms 151.6 282.6 227.2 228.58 43.674
Deselect 100k rows ms 133.1 271.8 192.5 196.28 48.515

Generated by 🚫 dangerJS against b693e02

@oliviertassinari oliviertassinari force-pushed the test-add-url-DOM branch 3 times, most recently from 8e3e92e to 56757ed Compare September 23, 2022 20:52
@oliviertassinari oliviertassinari mentioned this pull request Sep 23, 2022
Merged
4 tasks
@oliviertassinari oliviertassinari marked this pull request as ready for review September 23, 2022 21:27
@oliviertassinari oliviertassinari force-pushed the test-add-url-DOM branch 3 times, most recently from 19dab7e to 4f270f3 Compare September 23, 2022 22:44
oliviertassinari
oliviertassinari commented Sep 23, 2022
View reviewed changes
test/utils/createDOM.js
@@ -23,6 +23,7 @@ const blacklist = ['sessionStorage', 'localStorage'];
function createDOM() {
const dom = new JSDOM('', {
pretendToBeVisual: true,
url: 'http://localhost',
Copy link
Member Author

Choose a reason for hiding this comment

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

To prove that it has no effect. I don't know if we need this URL parameter to be honest, we could remove it.

test/regressions/TestViewer.js
@@ -6,7 +6,9 @@ import GlobalStyles from '@mui/material/GlobalStyles';
import { useLocation } from 'react-router-dom';
import { useFakeTimers } from 'sinon';

const StyledBox = styled(Box)(({ theme, isDataGridTest }) => ({
const StyledBox = styled(Box, {
shouldForwardProp: (prop) => prop !== 'isDataGridTest',
Copy link
Member Author

Choose a reason for hiding this comment

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

Leaking prop on the DOM node (React warning)

packages/grid/x-data-grid/src/hooks/features/export/useGridPrintExport.tsx
handlePrintWindowAfterPrint(printWindow);
};
};
doc.current!.body.appendChild(printWindow);
Copy link
Member Author

Choose a reason for hiding this comment

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

Set the load event listener after appending the child.

packages/grid/x-data-grid/src/hooks/features/export/useGridPrintExport.tsx
@@ -262,15 +266,21 @@ export const useGridPrintExport = (

await updateGridColumnsForPrint(options?.fields, options?.allColumns);
apiRef.current.unstable_disableVirtualization();
await raf(); // wait for the state changes to take action
Copy link
Member Author

Choose a reason for hiding this comment

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

This was already missing, but since we do no longer make a network request, the print displays a lot faster, which in the tests doesn't leave enough time for the API calls to render.

packages/grid/x-data-grid/src/hooks/features/export/useGridPrintExport.tsx
Comment on lines -104 to -106
// Some agents, such as IE11 and Enzyme (as of 2 Jun 2020) continuously call the
// `onload` callback. This ensures that it is only called once.
printWindow.onload = null;
Copy link
Member Author

Choose a reason for hiding this comment

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

IE 11 and enzyme are legacy, I'm not aware that we support them.

flaviendelangle
flaviendelangle approved these changes Sep 26, 2022
View reviewed changes
Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

No problem with dropping IE11 support if that's what the company is doing elsewhere.

packages/grid/x-data-grid/src/hooks/features/export/useGridPrintExport.tsx
@@ -54,6 +62,7 @@ export const useGridPrintExport = (
const updateGridColumnsForPrint = React.useCallback(
(fields?: string[], allColumns?: boolean) =>
new Promise<void>((resolve) => {
// TODO remove unused Promise
Copy link
Member

Choose a reason for hiding this comment

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

Are those TODO here because it's breaking ?
If so, we started the alpha phase so we can do the breaking change I think.

Copy link
Member Author

@oliviertassinari oliviertassinari Sep 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Are those TODO here because it's breaking?

The TODO is here because it would make it harder to review what changed in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense
We can make a follow up after that to remove the promise then

packages/grid/x-data-grid/src/hooks/features/export/useGridPrintExport.tsx
// Remove all loaded elements from the current host
printDoc.body.innerHTML = '';
printDoc.body.appendChild(gridClone);
// printDoc.body.appendChild(gridClone); should be enough but a clone isolation bug in Safari
Copy link
Member

Choose a reason for hiding this comment

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

If you have a link to the bug report to help future evolution of this method

Copy link
Member Author

@oliviertassinari oliviertassinari Sep 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I can't find it in https://bugs.webkit.org/buglist.cgi?quicksearch=cloneNode. However, I can provide the reproduction for the next person in x years that will blame the line, to give him confidence that the problem is gone.

Copy link
Member Author

Choose a reason for hiding this comment

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

On Safari 16.0, after a print without using innerHTML to cleanly deep clone:

Screenshot 2022-09-26 at 13 14 07

DanailH
DanailH approved these changes Sep 26, 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.

Nice cleanup

@oliviertassinari oliviertassinari mentioned this pull request Sep 26, 2022
Merged
@oliviertassinari oliviertassinari merged commit 54cd783 into mui:next Sep 27, 2022
@oliviertassinari oliviertassinari deleted the test-add-url-DOM branch September 27, 2022 10:19
cherniavskii pushed a commit to cherniavskii/mui-x that referenced this pull request Jan 6, 2023
@oliviertassinari @cherniavskii
[data grid] Improve print support (mui#6273)
ec29193
@cherniavskii cherniavskii mentioned this pull request Jan 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flaviendelangle flaviendelangle flaviendelangle approved these changes

@DanailH DanailH DanailH approved these changes

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Export performance test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[data grid] Print export scrolls the page to the bottom
4 participants
@oliviertassinari @mui-bot @flaviendelangle @DanailH