Cleaning and extract image comparison#55
Conversation
glibas
left a comment
glibas
left a comment
There was a problem hiding this comment.
Hi @BielinskiLukasz ,
Thank you for your contribution! Great work, never got around refactoring myself:)
Please find some comments on src/main/java/com/assertthat/selenium_shutterbug/core/PageSnapshot.java
and
src/main/java/com/assertthat/selenium_shutterbug/core/Snapshot.java
The rest looks great.
Regards,
Glib
| public PageSnapshot highlight(WebElement element, Color color, int lineWidth) { | ||
| try { | ||
| image = ImageProcessor.highlight(image, new Coordinates(element, devicePixelRatio), color, lineWidth); | ||
| ImageProcessor.highlight(image, new Coordinates(element, devicePixelRatio), color, lineWidth); |
There was a problem hiding this comment.
Need to keep assignment to image so subsequent chained methods can use updated image.
| try { | ||
| highlight(element, elementColor, 0); | ||
| Coordinates coords = new Coordinates(element, devicePixelRatio); | ||
| image = ImageProcessor.addText(image, coords.getX(), coords.getY() - textFont.getSize() / 2, text, textColor, textFont); |
There was a problem hiding this comment.
Need to keep assignment to image so subsequent chained methods can use updated image.
| * @return instance of type Snapshot | ||
| */ | ||
| public T monochrome() { | ||
| this.image = ImageProcessor.convertToGrayAndWhite(this.image); |
There was a problem hiding this comment.
Need to keep assignment to image so subsequent chained methods can use updated image.
|
Thanks for quick response and code review. What about @6de649e commit? I didn't test other combination but there is assignment scrollBarMaxWidth to _viewportHeight and next again scrollBarMaxWidth to _viewportWidth. I am wondering about the multiple uses of width. If it's correct, there is TODO what I created, should be removed :) |
|
Hi @BielinskiLukasz , Regarding @6de649e, I think we should leave this as is. scrollBarMaxWidth is standard so it is used for both - removing vertical and horizontal scrollbars if present. |
2 participants
Hello :)
I work with your code this week, great job. I made some cleaning there (removing doubles etc.). I extracted image comparison methods from ImageProcessor to a new class.
commit 6de649e
I'm not sure that the line is correct:
_viewportHeight -= scrollBarMaxWidth;