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

Implemented option for FullPage screenshot after the behaviours have run #656

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fservida
Copy link

@fservida fservida commented Jul 29, 2024

This PR implements a new option to take the FullPage screenshot after the behaviours have run instead of before.
This allows to capture elements that might not be fully loaded until the whole page is visited.

I only implemented for fullpage as I think it is the only screenshot where this is useful and the only case where I would need it, as it can include content not currently in frame. Feel free to expand on that as needed.

Related to #486

Tested with custom docker, though I had to ensure setuptools v71 as else it would not build.

@ikreymer
Copy link
Member

ikreymer commented Aug 2, 2024

Thanks for this - i think it would be clearer if it was part of the existing --screenshot flag, perhaps called fullPageAfterBehaviors - do you mind refactoring it to use that. That way can combine --screenshot fullPage,fullPageAfterBehaviors etc...

@fservida
Copy link
Author

fservida commented Oct 7, 2024

Reopening after adding changes

@fservida fservida reopened this Oct 7, 2024
@fservida
Copy link
Author

fservida commented Oct 7, 2024

@ikreymer I've reimplemented the changes as suggested, hope this can be useful

@ikreymer ikreymer requested a review from tw4l November 5, 2024 08:36
Copy link
Member

@tw4l tw4l left a comment

Choose a reason for hiding this comment

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

Making a new minor changes but overall looks good! Thank you for this.

src/crawler.ts Outdated Show resolved Hide resolved
src/util/argParser.ts Outdated Show resolved Hide resolved
@tw4l
Copy link
Member

tw4l commented Nov 5, 2024

@fservida Looks like we need to run the auto-formatter before merging to resolve linting issues. Would you mind pushing a commit to this branch after running yarn run format:fix? Thanks! :)

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.

3 participants