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

Removed shadow from iOS screencaps #72

Merged
merged 5 commits into from
May 11, 2017
Merged

Removed shadow from iOS screencaps #72

merged 5 commits into from
May 11, 2017

Conversation

jankcat
Copy link
Contributor

@jankcat jankcat commented May 3, 2017

I have removed the shadow from iOS screencaps.

The change essentially just takes slightly overlapping screenshots (scrolling to just above the previous screenshot's bottom edge, as opposed of directly at the edge of the previous screenshot), and then crops half the difference from the bottom of the first screenshot and from the top of the second screenshot.

I took the liberty of running it against my own suite as well, with the following results:

Without Fix With Fix iPhone 6 iOS 9 iPhone 7 iOS 10.2

@zinserjan
Copy link
Owner

Awesome! Thanks for this.

Just two things before this is ready to merge:

  • Can you add some tests for the calculation in TrimAndMergeScreenshotStrategy.js? For an example have a look at MergeScreenshotStrategy.test.js
  • Can you also remove the comment about the grey line in the readme?

@jankcat
Copy link
Contributor Author

jankcat commented May 3, 2017

For sure, let me take a look at that.

@jankcat
Copy link
Contributor Author

jankcat commented May 4, 2017

Aaand saucelabs is down, so the int tests are failing: https://status.saucelabs.com/incidents/sczxwt2zwz30

@jankcat
Copy link
Contributor Author

jankcat commented May 5, 2017

@zinserjan let me know if you want any more changes.

@jankcat
Copy link
Contributor Author

jankcat commented May 8, 2017

@zinserjan Any more comments?

@zinserjan
Copy link
Owner

@jankcat sorry, hadn't any time to review this, yet.

Looks good for me. Great job! I think this is ready to merge. But before I'll give it a try on a few projects, probably on wednesday evening.

@jankcat
Copy link
Contributor Author

jankcat commented May 8, 2017

Oh sure, fair enough. Let me know if it needs more work, I have time the end of this week to do so.

} else {
heightOffset = height - NAV_SHADOW_CONST;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Just thought if a if/else-if/else-if/else construct would make the cases more clearly. Maybe in the order "first AND last", "first only", "last only" and "between"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure let me take a looksie

@jankcat
Copy link
Contributor Author

jankcat commented May 10, 2017

@zinserjan I made the requested changes, I see what you mean about making it easier to understand.

@zinserjan
Copy link
Owner

@jankcat looks good. I'll merge this after the CI build is fixed (#74).

Question is how this should be released, as a patch (0.5.3) or as a minor/major (0.6.0)? This PR is kind of breaking change in case of the capturing results 😁

@jankcat
Copy link
Contributor Author

jankcat commented May 10, 2017

Haha thats a tough one... From a regression standpoint I would say 0.6.0. No apis changed but the output would break visual diffing for sure.

@zinserjan
Copy link
Owner

I would also say 0.6.0 to avoid visual regressions.

@zinserjan zinserjan merged commit 4e2385d into zinserjan:master May 11, 2017
@jankcat
Copy link
Contributor Author

jankcat commented May 12, 2017

Hurray! Thanks.

@jankcat
Copy link
Contributor Author

jankcat commented May 12, 2017

When will you do an npm publish?

@zinserjan
Copy link
Owner

wdio-screenshot 0.6.0 published 🎉 Great job @jankcat :)

I'll publish also a new version of wdio-visual-regression-service 😉

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