feat: add nativeElementsSize options #26
Conversation
aakapustin
changed the title
lib/config/index.js
Outdated
| @@ -42,6 +48,60 @@ const getParser = () => { | |||
| ? thr('Each browser must have "commands" option') | |||
| : assertArrayOfStrings(value, 'commands'); | |||
| } | |||
| }), | |||
| nativeElementsSize: section({ | |||
There was a problem hiding this comment.
I don't see any documentation for this option. Let's add it?
There was a problem hiding this comment.
Documentation added at README
lib/config/index.js
Outdated
| validate: (value, _config, currentNode) => { | ||
| if ( | ||
| (!_.isNull(value) && !_.isNumber(value)) || | ||
| (_.isNull(value) && !isNativeElementsSizeEmpty(currentNode)) |
There was a problem hiding this comment.
if one of the values is declared (for example bottomToolbarHeight) then in this case we will fall here with error "topToolbarHeight" must be a number but got ....
Why not just declare nativeElementsSize as option (without section) and check each option inside validate? You can look at this example - https://github.com/gemini-testing/hermione/blob/master/src/config/browser-options.js#L160-L173
There was a problem hiding this comment.
Ok, nativeElementsSize declaration is updated
lib/config/index.js
Outdated
| throw new Error(`"webviewWidth" must be a number but got ${value === null ? 'null' : typeof value}`); | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
To my mind these options must looks like:
nativeElementsSize: {
topToolbar: {width, height}, // or 100x200,
bottomToolbar: {width, height}, // or 100x200,
webview: {width, height}, // or 100x200,
}
This will reduce the number of options and if the width is needed, you will not need to add another option topToolbarWidth. What do you think?
Example how we validate windowSize in hermione - https://github.com/gemini-testing/hermione/blob/master/src/config/browser-options.js#L258-L274
| this._nativeElementsSize.topToolbarHeight && | ||
| this._nativeElementsSize.bottomToolbarHeight && | ||
| this._nativeElementsSize.webviewHeight && | ||
| this._nativeElementsSize.webviewWidth |
There was a problem hiding this comment.
Why you check every option like that? Looks like in you validate you can just check that this._nativeElementsSize is not empty.
| left: Math.max(Math.floor((webviewWidth - bodyWidth) / 2 * pixelRatio), 0), | ||
| top: Math.max(Math.floor(topToolbarHeight * pixelRatio), 0) | ||
| }; | ||
| } |
There was a problem hiding this comment.
Looks like we can invert the condition to reduce the amount of nested code
| ) { | ||
| let {topToolbarHeight, bottomToolbarHeight, webviewWidth, webviewHeight} = this._nativeElementsSize; | ||
|
|
||
| if (await browser.getOrientation() === 'LANDSCAPE') { |
aakapustin
force-pushed
the
HERMIONE-1405.expected_native_elements_size
branch
from
db564a7
to
096abe9
Compare
| height: 654, | ||
| width: 390 | ||
| } | ||
| } |
There was a problem hiding this comment.
Looks like we will need to choose default values for each version of safari in some next releases. In order not to force the user to specify them.
README.md
Outdated
| @@ -20,6 +20,17 @@ Plugin has following configuration: | |||
| * **enabled** (optional) `Boolean` – enable/disable the plugin, by default plugin is enabled; | |||
| * **browsers** (required) `Object` - the list of browsers to use for wrap commands; | |||
| * **commands** (required) `Array` - commands which will be wrapped. | |||
| * **nativeElementsSize** (optional) `Object` - the map of native elements sizes for the browser; | |||
There was a problem hiding this comment.
Can we specify that the values should be specified without taking into account the pixel ratio?
I would also add that when specifying these values, the speed of taking screenshots increases.
|
|
||
| describe('"calcWebViewCoordsNative" method', () => { | ||
| it('should use _nativeElementsSize if exists', async () => { | ||
| sinon.stub(utils, '_nativeElementsSize').value({ |
There was a problem hiding this comment.
We don't use private fields in test files. Here you just need to call mkUtilsStub with the necessary arguments (send nativeElementsSize as second argument`).
I didn't notice this problem at the first review.
aakapustin
force-pushed
the
HERMIONE-1405.expected_native_elements_size
branch
from
096abe9
to
db05d6f
Compare
DudaGod
force-pushed
the
HERMIONE-1405.expected_native_elements_size
branch
from
db05d6f
to
56a7091
Compare
Added option nativeElementsSize to define the expected sizes of native elements. This will prevent switching to the native context before taking a screenshot