Emoji search on mobile

[perunt]

Details

Fixed Issues

$ #14098
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

All Platforms:

1. Create a new account.
2. Open a new chat conversation.
3. Access the emoji picker by clicking on the emoji icon in the chat input field.
4. Make sure the search bar is visible at the top of the emoji picker.
5. Type "hand" in the search bar and verify that the list of emojis is updated to show relevant hand emojis.
6. Change the skin tone of an emoji using the appropriate option.
7. Verify that the skin tone of the selected emoji is updated.
8. Type the random string "handsmsmsm" in the search bar.
9. Verify that the message "No results found" is displayed.
10. Clear the search bar by deleting all text.
11. Verify that the emoji picker returns to its starter state with all emojis visible and the search bar empty.

• Verify that no errors appear in the JS console

Offline tests

QA Steps

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mar-01-2023.10-26-38.mp4

Mobile Web - Chrome

Mar-01-2023.10-19-07.mp4

Mobile Web - Safari

Mar-01-2023.10-14-19.mp4

Desktop

Mar-01-2023.10-25-49.mp4

iOS

Mar-01-2023.10-33-15.mp4

Mar-01-2023.10-34-36.mp4

Android

Mar-01-2023.10-34-36.mp4

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[melvin-bot]

@tgolen Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[stitesExpensify]

Looking pretty good! After reviewing I got the thought though, what are the differences between index.js and the index.native.js now? Do we think we could re-unify them?

[stitesExpensify]

@perunt what do you think of my question here about potentially re-unifying web and native now that they have the same features? Is there enough significant difference to keep them separate?

[perunt]

@stitesExpensify I agree, it would be great if we could unify them.
I think it makes sense to wait until your PR is merged, then, we can reevaluate and see if there's any need for additional changes. But if you'd like, I can also take a closer look at your PR to see if we can avoid duplicating efforts

[stitesExpensify]

My PR was just merged, let me know what you think now!

[perunt]

@stitesExpensify after testing this branch with your implementation, I noticed an issue with the UX on some small screens. Specifically, on devices like the iPhone SE/13 mini, the input is still readable but slightly cut off. On Android, such as the Pixel 2, the input is positioned under the status bar. How small of a screen do we need to be concerned about?

[stitesExpensify]

I would say we need to support all popular device types, including the mini.

[luacmartins]

LGTM and tests well. Left a couple of NAB comments

[luacmartins]

I'm surprised we still have isMobileLandscape since we disable landscape mode on small screens. Maybe we can create a follow up issue to clean this up.

[stitesExpensify]

Code looks great and it tests well, let's !

[stitesExpensify]

One question, how are users supposed to close the picker if they don't want to pick anything?

2023-03-23_13-04-57.mp4

If you click the very top it will go away, but it definitely feels like you should be able to swipe that menu down. While testing I felt "trapped" on that screen. Thoughts @luacmartins @perunt ?

[perunt]

@stitesExpensify currently, there are three options to close the element:

1. Swipe back or press the back button.
2. Hide the keyboard and tap on an empty space.
3. Select an emoji.
   However, instead of simply adding a close button, I'd recommend considering a redesign of this element to improve the user experience. One potential solution is to implement a bottom sheet component for mobile devices. Here's a popular React Native bottom sheet library that would be good for it: https://github.com/gorhom/react-native-bottom-sheet#react-native-bottom-sheet

[luacmartins]

Interesting. I didn't feel trapped, but from the video I can see how bad it is for small screen devices. I agree that a swipe down to dismiss would be good.

[stitesExpensify]

Okay well if others aren't feeling trapped right now, let's go ahead and merge it, and then we can change it later if I'm not the only one!

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[bernhardoj]

Hi, there is a regression reported here from this PR. Removing the KeyboardAvoidingView will solve the regression, but I'm aware that it's added to resize the modal (especially for emoji picker) when keyboard shows.

If I may suggest, instead of using KeyboardAvoidingView, we can just set statusBarTranslucent to false for emoji picker only here.

App/src/components/EmojiPicker/EmojiPicker/index.js

Lines 168 to 170 in d57b331

	<PopoverWithMeasuredContent
	isVisible={this.state.isEmojiPickerVisible}
	onClose={this.hideEmojiPicker}

statusBarTranslucent true (which is the default value of our modal) will make the modal does not get resized when keyboard shows. More info here #15373 (comment)

[situchan]

This PR caused regression - #16636
I have a solution to fix without reverting this big PR.

Proposal

Please re-state the problem that we are trying to solve in this issue.

attachment modal views are broken

What is the root cause of that problem?

App/src/components/Modal/BaseModal.js

Lines 175 to 181 in 752ac61

	<KeyboardAvoidingView
	behavior="padding"
	style={styles.w100}
	shouldApplyToAndroid
	>
	{content}
	</KeyboardAvoidingView>

KeyboardAvoidingView height is not stretched. Neither flex: 1 nor height: '100%' is added in style.
But we can't simply add these styles because emoji picker doesn't need to be stretched.

What changes do you think we should make in order to solve the problem?

• completely remove android specific KeyboardAvoidingView introduced in this PR
• revert changes in BaseModal (return normal view as before)
• pass avoidKeyboard={true} to PopoveWithMeasuredContent in EmojiPicker component
  this is to push emoji picker above keyboard on iOS
• pass statusBarTranslucent={false} to PopoveWithMeasuredContent in EmojiPicker component
  this is to push emoji picker above keyboard on Android

[OSBotify]

🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.2.91-0 🚀

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

[cristipaval]

This PR introduced the following deploy blockers:

• [HOLD for payment 2023-04-07] App Crashing when sending attachments #16641
• [HOLD for payment 2023-04-07] [$1000] DEV: Preview of the image is showing broken and does not display image #16636

[luacmartins]

Here's the full list of deploy blockers introduced by this PR:

• [HOLD for payment 2023-04-07] [$1000] DEV: Preview of the image is showing broken and does not display image #16636
• [HOLD for payment 2023-04-07] [HOLD] The "Delete Comment" context tab has been moved to the left #16639
• [HOLD for payment 2023-04-07] App Crashing when sending attachments #16641
• [$1000] [HOLD for payment 2023-04-07] mWeb - Chat - If you edit a message using emoji, emoji picker and keyboard opens. #16653

[stitesExpensify]

Thank you, my apologies for not catching those. According to @perunt this PR will fix some of them, and when we re-implement the feature, we will make sure that none of those bugs are happening anymore

[OSBotify]

🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.91-1 🚀

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