Skip to content

Add ability to manually save screenshots with custom prefixes #256

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mtancoigne
Copy link

I like to make custom screenshots for demonstration purpose, so this PR updates screenshot_and_save_page and screenshot_and_open_image with two additional parameters: prefix and html

This is useful to differentiate screenshots from failures.

Copy link
Owner

@mattheworiordan mattheworiordan left a comment

Choose a reason for hiding this comment

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

I appreciate the contribution, but please see my comments in regards to changes I would require to merge this I am afraid. Shout if you want to discuss any of this.

README.md Outdated
screenshot_and_save_page('custom_prefix')

# image only, with custom prefix:
screenshot_and_save_page('custom_prefix', true)
Copy link
Owner

Choose a reason for hiding this comment

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

I am afraid I cannot approve this. I am happy to support this PR, however we need to see;

  • Use of options in this method so that adding new options in future is more easily supported, and the code is more readable. For example, I'd expect to see screenshot_and_save_page(prefix: 'custom_prefix', html: true).
  • I am not sure even the former arg html makes sense. What does that even mean, given your comment says "image only" and the default is true?
  • I'd like to see this approach with argument options = {} carried through internal methods
  • We need to have some tests for this, even if very basic, so that changes in future won't introduce regressions.

@mtancoigne
Copy link
Author

No problem, I'll try to fix this this week.

@mtancoigne
Copy link
Author

Hi !

For the arguments, do you prefer options or keyword arguments ?

For html, this was a mistake on my side in the README, it should have been false. I set this option so one can save only image screenshots without breaking the way the library works today.

@mattheworiordan
Copy link
Owner

For the arguments, do you prefer options or keyword arguments ?

Well options because we still support older versions of Ruby, for now.

@mtancoigne
Copy link
Author

Hi! I think i'm ready with the changes.

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

Successfully merging this pull request may close these issues.

2 participants