-
Notifications
You must be signed in to change notification settings - Fork 43
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
Adding support for Android devices #79
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,3 +9,6 @@ yarn.lock | |
# project | ||
.tmp | ||
lib | ||
|
||
# Mac OSX | ||
.DS_Store | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,7 +83,7 @@ export default class ScreenDimensions { | |
} | ||
|
||
getScale() { | ||
if (this.isIOS) { | ||
if (this.isIOS || this.isAndroid) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a test case for this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me see if I can add some off of the existing ones |
||
return this.getScreenWidth() / this.getViewportWidth(); | ||
} | ||
return 1; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,8 +26,8 @@ export default class ScreenshotStrategyManager { | |
return new FullpageScreenshotStrategy(screenDimensions); | ||
} | ||
|
||
const { isIOS } = browser; | ||
if (isIOS) { | ||
const { isMobile } = browser; | ||
if (isMobile) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this really necessary for android simulators or in other words, is merging enough? iOS uses this to remove the grey line between the stiched screenshots. For real android devices with chrome this shouldn't be executed, merging ist enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, let me check just using the merge strategy on Android. |
||
log('use iOS Trim and Merge viewport strategy') | ||
return new TrimAndMergeViewportStrategy(screenDimensions); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
export default { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delete this file. Have a look at my other comment for the reason. |
||
android: { | ||
toolbarHeight: 43, | ||
addressbarHeight: 77 | ||
}, | ||
iOS: { | ||
toolbarHeight: 44, | ||
addressbarHeight: 44 | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import CropDimension from './CropDimension'; | ||
import getBase64ImageSize from './getBase64ImageSize'; | ||
import { cropImage, scaleImage } from './image'; | ||
import fixedHeights from './mobile/fixedHeights'; | ||
|
||
async function normalizeRetinaScreenshot(browser, screenDimensions, base64Screenshot) { | ||
// check if image dimensions are different to viewport as browsers like firefox scales images automatically down | ||
|
@@ -18,16 +19,19 @@ async function normalizeRetinaScreenshot(browser, screenDimensions, base64Screen | |
return base64Screenshot; | ||
} | ||
|
||
async function normalizeIOSScreenshot(browser, screenDimensions, base64Screenshot) { | ||
const toolbarHeight = 44; // bottom toolbar has always a fixed height of 44px | ||
const addressbarHeight = 44; // bottom toolbar has always a fixed height of 44px | ||
async function normalizeMobileScreenshot(browser, screenDimensions, base64Screenshot) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please create a seperate normalize function for Android. Using a normalizer that does iOS and android at the same time is just complicated and uncecessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. |
||
const mobileFixedHeights = (browser.isAndroid) ? fixedHeights.android : fixedHeights.iOS; | ||
const toolbarHeight = mobileFixedHeights.toolbarHeight; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does Android really have a toolbar at the bottom? I can't see any at your screenshots. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uiii, this doesn't look like a case that is fixable. Especially with the offsets around the device screenshot and the android controls on the right side 😁 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, this is a screenshot from the saucelabs video 😅 |
||
const addressbarHeight = mobileFixedHeights.addressbarHeight; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the height of the adressbar is something that we can calculate with In my opinion this makes all static height values unnecessary, am I wrong? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue is that toolbar appears only in some cases. Is there a way to get the y offset of the viewport from the top of the screen? If so then I can calculate whether the footer appears. I think if I use |
||
|
||
const viewportHeight = screenDimensions.applyScaleFactor(screenDimensions.getViewportHeight()); | ||
const viewportWidth = screenDimensions.applyScaleFactor(screenDimensions.getViewportWidth()); | ||
let isIpad = false; | ||
|
||
// all iPad's have 1024.. | ||
const isIpad = screenDimensions.getScreenHeight() === 1024 || screenDimensions.getScreenWidth() === 1024; | ||
const isIphone = !isIpad; | ||
if (browser.isIOS) { | ||
isIpad = screenDimensions.getScreenHeight() === 1024 || screenDimensions.getScreenWidth() === 1024; | ||
} | ||
|
||
// detect if status bar + navigation bar is shown | ||
const barsShown = viewportHeight < screenDimensions.getScreenHeight(); | ||
|
@@ -37,7 +41,7 @@ async function normalizeIOSScreenshot(browser, screenDimensions, base64Screensho | |
// calculate height of status + addressbar | ||
barsHeight = screenDimensions.getScreenHeight() - viewportHeight; | ||
|
||
if (isIphone && barsHeight > addressbarHeight) { | ||
if (!isIpad && barsHeight > addressbarHeight) { | ||
// iPhone's have also sometimes toolbar at the bottom when navigation bar is shown, need to consider that | ||
barsHeight -= toolbarHeight; | ||
} | ||
|
@@ -58,18 +62,18 @@ async function normalizeIOSScreenshot(browser, screenDimensions, base64Screensho | |
return base64Screenshot; | ||
} | ||
|
||
|
||
export default async function normalizeSreenshot(browser, screenDimensions, base64Screenshot) { | ||
export default async function normalizeScreenshot(browser, screenDimensions, base64Screenshot) { | ||
let normalizedScreenshot = base64Screenshot; | ||
|
||
// check if we could have a retina image | ||
if (screenDimensions.getPixelRatio() > 1) { | ||
normalizedScreenshot = await normalizeRetinaScreenshot(browser, screenDimensions, normalizedScreenshot); | ||
} | ||
|
||
// check if we have to crop navigation- & toolbar for iOS | ||
if (browser.isMobile && browser.isIOS) { | ||
normalizedScreenshot = await normalizeIOSScreenshot(browser, screenDimensions, normalizedScreenshot); | ||
// check if we have to crop navigation- & toolbar for iOS or Android | ||
if (browser.isMobile) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As already said, please separate iOS from android here for maintenance reasons. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to make sure that you doesn't break real android devices with chrome here. They don't need additional cropping as the screenshots are the same like the desktop chrome. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm, does the |
||
normalizedScreenshot = await normalizeMobileScreenshot(browser, screenDimensions, normalizedScreenshot); | ||
} | ||
|
||
return normalizedScreenshot; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"body": { | ||
"offsetHeight": 0, | ||
"scrollHeight": 480 | ||
}, | ||
"window": { | ||
"pixelRatio": 2, | ||
"orientation": 0, | ||
"innerWidth": 320, | ||
"screenHeight": 480, | ||
"innerHeight": 403, | ||
"screenWidth": 320 | ||
}, | ||
"html": { | ||
"clientWidth": 320, | ||
"offsetHeight": 0, | ||
"scrollWidth": 320, | ||
"clientHeight": 480, | ||
"scrollHeight": 0 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"body": { | ||
"offsetHeight": 0, | ||
"scrollHeight": 480 | ||
}, | ||
"window": { | ||
"pixelRatio": 2, | ||
"orientation": 0, | ||
"innerWidth": 320, | ||
"screenHeight": 480, | ||
"innerHeight": 403, | ||
"screenWidth": 320 | ||
}, | ||
"html": { | ||
"clientWidth": 320, | ||
"offsetHeight": 0, | ||
"scrollWidth": 320, | ||
"clientHeight": 480, | ||
"scrollHeight": 0 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"body": { | ||
"offsetHeight": 0, | ||
"scrollHeight": 480 | ||
}, | ||
"window": { | ||
"pixelRatio": 2, | ||
"orientation": 0, | ||
"innerWidth": 320, | ||
"screenHeight": 480, | ||
"innerHeight": 403, | ||
"screenWidth": 320 | ||
}, | ||
"html": { | ||
"clientWidth": 320, | ||
"offsetHeight": 0, | ||
"scrollWidth": 320, | ||
"clientHeight": 480, | ||
"scrollHeight": 0 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"body": { | ||
"offsetHeight": 0, | ||
"scrollHeight": 480 | ||
}, | ||
"window": { | ||
"pixelRatio": 2, | ||
"orientation": 0, | ||
"innerWidth": 320, | ||
"screenHeight": 480, | ||
"innerHeight": 403, | ||
"screenWidth": 320 | ||
}, | ||
"html": { | ||
"clientWidth": 320, | ||
"offsetHeight": 0, | ||
"scrollWidth": 320, | ||
"clientHeight": 480, | ||
"scrollHeight": 0 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"body": { | ||
"offsetHeight": 0, | ||
"scrollHeight": 480 | ||
}, | ||
"window": { | ||
"pixelRatio": 2, | ||
"orientation": 0, | ||
"innerWidth": 320, | ||
"screenHeight": 480, | ||
"innerHeight": 403, | ||
"screenWidth": 320 | ||
}, | ||
"html": { | ||
"clientWidth": 320, | ||
"offsetHeight": 0, | ||
"scrollWidth": 320, | ||
"clientHeight": 480, | ||
"scrollHeight": 0 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"body": { | ||
"offsetHeight": 0, | ||
"scrollHeight": 480 | ||
}, | ||
"window": { | ||
"pixelRatio": 2, | ||
"orientation": 0, | ||
"innerWidth": 320, | ||
"screenHeight": 480, | ||
"innerHeight": 403, | ||
"screenWidth": 320 | ||
}, | ||
"html": { | ||
"clientWidth": 320, | ||
"offsetHeight": 0, | ||
"scrollWidth": 320, | ||
"clientHeight": 480, | ||
"scrollHeight": 0 | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OS dependent stuff should be ignored in a global .gitignore file, but I'm fine with merging this in to avoid this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by a global
.gitignore
, I presumed that was this.