Get ios version number in platform #8173
Conversation
|
Hi, ampproject bot here! Here are a list of the owners that can approve your files. You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status /to dvoytenko jridgewell
/to aghassemi alanorozco camelburrito chenshay choumx cramforce cvializ dvoytenko ericlindley-g erwinmombay gregable honeybadgerdontcare jridgewell kmh287 lannka mkhatib mrjoro muxin newmuis powdercloud zhouyx
For any issues please file a bug at https://github.com/google/github-owners-bot/issues |
src/service/platform-impl.js
Outdated
| * Direct string equality check is not suggested, use startWith instead. | ||
| * @returns {string} | ||
| */ | ||
| getIosVersion() { |
There was a problem hiding this comment.
you should do a getMinorVersion instead. is that too complicated?
@dvoytenko could you take a look pls
There was a problem hiding this comment.
getMajorVersion is for browser major version. I thought if we have a getMinorVersion, that should be for browser too, but there's no such use case yet. Also, there's no use case for getting android version, so that's why I only created getIosVersion
There was a problem hiding this comment.
Let's just name getIosVersionString().
|
Hi, ampproject bot here! Here are a list of the owners that can approve your files. You may leave an issue comment stating "@ampprojectbot retry!" to force me to re-evaluate this Pull Request's status /to dvoytenko jridgewell
/to alanorozco camelburrito chenshay choumx cramforce cvializ dvoytenko ericlindley-g erwinmombay gregable honeybadgerdontcare jridgewell kmh287 lannka mkhatib mrjoro muxin newmuis powdercloud zhouyx
For any issues please file a bug at https://github.com/google/github-owners-bot/issues |
src/service/platform-impl.js
Outdated
| if (!this.navigator_.userAgent) { | ||
| return ''; | ||
| } | ||
| let version = this.navigator_.userAgent |
There was a problem hiding this comment.
Do we need to first test that this is indeed iOS? E.g. if (!isIos()) {return '';}?
There was a problem hiding this comment.
From what I see in the user agent string, iPhone|iPad|iPod is always followed by OS 10_3. But it doesn't hurt to check. I'll check it.
There was a problem hiding this comment.
Sure. But there could be "OS ???" in other browsers as well. Since the method is called getIosVersionString, we should make sure it's indeed iOS.
There was a problem hiding this comment.
From what I see, for ios it's always iPhone OS 10_3 together, haven't seen any other platform using OS ???. Anyway I added the check.
|
Wait for @dvoytenko to approve |
|
Merging it because it's blocking #7670. @dvoytenko If there's any other comments I can address later. |
Needed in #7670 and #7653
/cc @dvoytenko