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

Lazy load report images #825

Merged
merged 6 commits into from
Jul 23, 2018
Merged

Conversation

justinph
Copy link
Contributor

This change makes viewing the generated HTML reports more efficient and user-friendly by not loading test images that are not yet in the viewport. This makes it far more practical to view large reports quickly and not have the page consume large amounts of bandwidth.

This change adds and uses react-visibility-sensor to handle the visibility detection. This is the library used on www.nytimes.com to lazily load images.

Note: please ignore the extra early commits in my PR from some earlier work. I assume this will get squash-merged so they won't matter.

Copy link
Owner

@garris garris left a comment

Choose a reason for hiding this comment

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

LATM (looks awesome to me)

@garris garris merged commit aa6189a into garris:master Jul 23, 2018
@garris
Copy link
Owner

garris commented Jul 23, 2018

@justinph Thanks for the PR -- I have been wanting to see this for a long time! Finally! 😄 I will check this before pushing to NPM -- but before I do I just want to make sure you checked this against npm run smoke-test. And also did you happen to test this with different display states? (eg...
viewing all pass, all fail, changing search criteria and excluding diff, and/or reference images in report via the control to the right of the search window) Please let me know. Thanks super a lot!

@justinph
Copy link
Contributor Author

@garris: Yup, it looked all good from the smoke tests. Also ran it against our own configs and it works well, no discernible issues.

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.

2 participants