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

Chrome full size screenshot without sticky header #38

Merged
merged 5 commits into from
Jul 16, 2018

Conversation

anna-shchurok
Copy link
Collaborator

No description provided.

Copy link
Member

@glibas glibas left a comment

Choose a reason for hiding this comment

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

Hi @galkina-anna ,

Thank you very much for your contribution. Could you please take a look at couple comments before PR can be merged.

Also could you please update docs with new usage examples.

Regards,
G

/**
* Created by Anna Galkina on 23-06-2018.
*/
public class DriverCommandExecutor {
Copy link
Member

Choose a reason for hiding this comment

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

Lets move command execution methods to Browser class

@@ -0,0 +1,3 @@
({width: Math.max(window.innerWidth,document.body.scrollWidth,document.documentElement.scrollWidth)|0,
height: Math.max(window.innerHeight,document.body.scrollHeight,document.documentElement.scrollHeight)|0,
deviceScaleFactor: window.devicePixelRatio || 1,mobile: typeof window.orientation !== 'undefined'})
Copy link
Member

Choose a reason for hiding this comment

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

Add newline at the end of file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New line added, but here in PR it is not showed:
image

@@ -97,7 +97,7 @@ public static PageSnapshot shootPage(WebDriver driver, ScrollStrategy scroll, in
case BOTH_DIRECTIONS:
pageScreenshot.setImage(browser.takeScreenshotEntirePage());
case WITHOUT_STICKY_HEADER:
Copy link
Member

Choose a reason for hiding this comment

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

Lets call it WHOLE_PAGE_CHROME

@@ -97,7 +97,7 @@ public static PageSnapshot shootPage(WebDriver driver, ScrollStrategy scroll, in
case BOTH_DIRECTIONS:
pageScreenshot.setImage(browser.takeScreenshotEntirePage());
case WITHOUT_STICKY_HEADER:
pageScreenshot.setImage(browser.takeScreenshotEntirePage(true));
pageScreenshot.setImage(browser.takeScreenshotEntirePageUsingChromeCommand());
Copy link
Member

Choose a reason for hiding this comment

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

Shall we add broser.getName(); which will check which driver instance is currently used (instanceof presumably) and throw UnsupportedOperationException if current driver instance is other than Chrome

Copy link
Member

Choose a reason for hiding this comment

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

Also, please add missing break to BOTH_DIRECTIONS case

Object metrics = driver.evaluate(FileUtil.getJsScript(ALL_METRICS));
driver.sendCommand("Emulation.setDeviceMetricsOverride", metrics);
Object result = driver.sendCommand("Page.captureScreenshot", ImmutableMap.of("format", "png", "fromSurface", true));
driver.sendCommand("Emulation.clearDeviceMetricsOverride", ImmutableMap.of());
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like we need to clearDeviceMetricsOverride, as we don't really changing any initial device metrics just width and height which wont influence further test flow, moreover clearDeviceMetricsOverride for some reason spoins further highlighting/bluring etc. elements (coordinates are shifted).

@glibas
Copy link
Member

glibas commented Jul 9, 2018

Update from master before resolving.

Copy link
Collaborator Author

@anna-shchurok anna-shchurok left a comment

Choose a reason for hiding this comment

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

Hi @glib-briia !

Thank you a lot for your review!

I have fixed my code following your comments. Only one left - I didn't change the doc as on README file this information would be inappropriate and wiki I have no access to amend.

Could you please do this for me or tell me how I find the way to change the doc?

Thank you,

@glibas glibas merged commit 00a4456 into assertthat:master Jul 16, 2018
@anna-shchurok anna-shchurok deleted the feature-fullsize-screenshot branch July 17, 2018 15:53
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