-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add ability to get the current operating system, display subset of emojis if on Windows #2787
Conversation
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.
Looks great, thanks!
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.
Changing my review until we chat about splitting the lib
@stitesExpensify Replied your comment, I decided it's better to split it into two files to keep things more consistent with other library functions that have web and native counterparts. We should be good for another review! |
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.
Thanks for making those changes, looks great!
🚀 Deployed to staging in version: 1.0.42-8🚀
|
* @return {String | null} | ||
*/ | ||
export default () => { | ||
switch (Platform.OS) { |
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.
We usually split stuff like this into an index.android.js
and index.ios.js
files instead of use Platform
+ index.native.js
.
const {userAgent, platform} = window.navigator; | ||
const macosPlatforms = ['Macintosh', 'MacIntel', 'MacPPC', 'Mac68K']; | ||
const windowsPlatforms = ['Win32', 'Win64', 'Windows', 'WinCE']; | ||
const iosPlatforms = ['iPhone', 'iPad', 'iPod']; |
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.
NAB, if we only needed to know that we're on windows we could have created an isUsingWindows
helper instead.
🚀 Deployed to production in version: 1.0.44-0🚀
|
Details
Windows doesn't support flag emojis (and just displays country codes instead), so let's prevent those emojis from displaying in the emoji picker if we're on Windows.
To do this, I added a function
getOperatingSystem
that reads the current operating system.Fixed Issues
Fixes #2761
Tests
For the
getOperatingSystem
function, I logged its results for all operating systems and verified that they were correct.I then verified that flag emojis don't appear in the emoji picker on Windows.
QA Steps
Tested On
Screenshots
Web
1620717907646955.mp4