Feature: Add env.isEdge property. #248
Conversation
src/env.js
Outdated
| * @returns {Boolean} Whether User Agent is Edge or not. | ||
| */ | ||
| export function isEdge( userAgent ) { | ||
| return userAgent.indexOf( 'edge' ) > -1; |
There was a problem hiding this comment.
That's unsafe. There are many words with "edge" in them. Something more precise would be better.
There was a problem hiding this comment.
BTW, feel free to look into CKE4's code:
https://github.com/ckeditor/ckeditor-dev/blob/master/core/env.js#L20
There was a problem hiding this comment.
@Reinmar I'm not 100% sure but it looks like [ /] in that regex could be restricted to:
/edge\/(\d+.?\d*)/I couldn't find why there is space allowed...
There was a problem hiding this comment.
Describes only Edge/<EdgeHTML Rev>.<Windows Build> as possible value so I've set the regex accordingly to this.
| @@ -31,4 +37,19 @@ describe( 'Env', () => { | |||
| expect( isMac( 'foo' ) ).to.be.false; | |||
| } ); | |||
| } ); | |||
|
|
|||
| describe( 'isEdge()', () => { | |||
| it( 'returns true for Edge UA strings', () => { | |||
There was a problem hiding this comment.
Yeah - I've copied from other test. I'm adding some real ones right now ;)
There was a problem hiding this comment.
OK... I was too slow 😞
Suggested merge commit message (convention)
Feature: Introduce
env.isEdgeproperty.Additional information