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

SLVUU-101 improve take screenshot functionality #124

Closed

Conversation

vferraro-scottlogic
Copy link

@vferraro-scottlogic vferraro-scottlogic commented Nov 28, 2023

There is an open BUG tracking this issue on the html-to-image repo. The following changes are aimed at mitigating the issue with the UI freezing when taking a screenshot of a very complex layout

Changes

  • When taking the screenshot, filter DOM elements out based on role (exclude the content of every element that has role="row")
  • add spinner while image is loading
  • takeScreenshot function will reject with an error message if something went wrong while taking the screenshot
  • SaveLayoutDialog will render:
    • a screenshot if screenshot state is not undefined
    • an error message if screenshotErrorMessage is not undefined
    • or else a spinner

Before
2
After
1
takeScreenshot function throws error (in this case the error is thrown because the layout is not displayed in the DOM)
3

Note

  • Filtering out the row elements means we won't be displaying the content of the tables inside the screenshot which could be a positive thing if we are worried about the data being sensitive

  • fix unit tests

@vferraro-scottlogic vferraro-scottlogic linked an issue Nov 28, 2023 that may be closed by this pull request
@pling-scottlogic
Copy link

Could you add some static images of equivalent screenshots before and after the change, please? It's a bit tricky to evaluate from the gifs.

Comment on lines +13 to +14
child.nodeType === Node.TEXT_NODE ||
child.getAttribute("role") !== "row",

Choose a reason for hiding this comment

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

These look like good filter criteria to me, but I'd like to know what @heswell thinks.

@vferraro-scottlogic
Copy link
Author

Could you add some static images of equivalent screenshots before and after the change, please? It's a bit tricky to evaluate from the gifs.

Here'a an example of the layout screenshot before and after the change:
before
after

Copy link

@pling-scottlogic pling-scottlogic left a comment

Choose a reason for hiding this comment

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

I'm happy to approve this, but I feel like the styling could be improved where we now have empty rectangles for tables. Perhaps by including a placeholder image to represent a table, and removing the scrollbars.

@vferraro-scottlogic
Copy link
Author

raised PR to finos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long delay to open save layout dialog
3 participants