Skip to content
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

Merged
merged 9 commits into from
May 12, 2021

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented May 11, 2021

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

  1. On a Windows laptop, sign in, and navigate to any chat.
  2. Open the emoji picker. Scroll down to the bottom and verify that there is no "Flags" category.

Tested On

  • Web

Screenshots

Web

1620717907646955.mp4

@jasperhuangg jasperhuangg self-assigned this May 11, 2021
@jasperhuangg jasperhuangg requested a review from a team May 11, 2021 08:02
@MelvinBot MelvinBot requested review from stitesExpensify and removed request for a team May 11, 2021 08:02
@jasperhuangg jasperhuangg marked this pull request as ready for review May 11, 2021 11:31
@jasperhuangg jasperhuangg requested a review from a team as a code owner May 11, 2021 11:31
@MelvinBot MelvinBot removed the request for review from a team May 11, 2021 11:31
Copy link
Contributor

@stitesExpensify stitesExpensify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

Copy link
Contributor

@stitesExpensify stitesExpensify left a 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

@jasperhuangg
Copy link
Contributor Author

@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!

Copy link
Contributor

@stitesExpensify stitesExpensify left a 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!

@stitesExpensify stitesExpensify merged commit 128309c into main May 12, 2021
@stitesExpensify stitesExpensify deleted the jasper-fixEmojiFlagsWindows branch May 12, 2021 22:56
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.42-8🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@isagoico
Copy link

image

It's a pass! no flag category.

* @return {String | null}
*/
export default () => {
switch (Platform.OS) {
Copy link
Contributor

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'];
Copy link
Contributor

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.

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.44-0🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emojis - Country codes are displayed instead of corresponding flag emojis
5 participants