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

[Feat]: Emoji Picker #8157

Open
wants to merge 54 commits into
base: main
Choose a base branch
from
Open

[Feat]: Emoji Picker #8157

wants to merge 54 commits into from

Conversation

Rajat-Dabade
Copy link
Contributor

Summary

This PR adds a feature for the emoji picker in message input.

Ticket Link

https://mattermost.atlassian.net/browse/MM-53959

Device Information

IOS, Andriod and Ipad

Screenshots

Release Note

Added a new feat to select emoji from emoji picker for message input.

@Rajat-Dabade Rajat-Dabade self-assigned this Aug 20, 2024
@Rajat-Dabade Rajat-Dabade marked this pull request as ready for review August 27, 2024 11:31
@Rajat-Dabade Rajat-Dabade added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester 2: UX Review Requires review by a UX Designer labels Aug 27, 2024
@yasserfaraazkhan
Copy link
Contributor

/update-branch

@yasserfaraazkhan yasserfaraazkhan added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Aug 27, 2024
Copy link
Contributor

@yasserfaraazkhan yasserfaraazkhan left a comment

Choose a reason for hiding this comment

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

Hi @Rajat-Dabade
The functionality of selecting emojis works well in center channel, threads.

There's one observation I want to share. We have to click on the emoji icon 2 times to have it display the emoticons. 1st click will always opens the keyboard and 2nd click opens the emoji picker.
Please let me know if this is expected.
https://github.com/user-attachments/assets/9a37e0a9-f074-461c-9e22-91f53635fa85

Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

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

Thanks @Rajat-Dabade, great work!

We have to click on the emoji icon 2 times to have it display the emoticons. 1st click will always opens the keyboard and 2nd click opens the emoji picker.
Please let me know if this is expected.

@yasserfaraazkhan This is not expected and it should open the emoji picker in the first tap. Also, I see it working fine in iOS though so this might be specific to Android.

There are a few other issues I noticed:

  1. The button size is too large for the icon buttons. It seems to be 46pt x 46pt whereas it should be 40x40 as per the design. The circle around the + button is also not fully rounded. I believe the space around the icons needs to be reduced.
  2. Borders above and below the search bar are missing.
  3. The divider just above the footer containing category icon buttons should be edge to edge.
  4. There is too much space above and below the category icon button row. It can be reduced to match the design.
  5. The tap area for the keyboard and backspace button seems to be smaller than the category icon buttons. It should match with the category button. Also the grey used in the on-tap state seems too dark.
image
  1. The animation to switch between the keyboard and the emoji picker seems weird right now. Is there a way to not move the message scroll area content while switching between these? Ideally it should be similar to what can be seen in this prototype.
Screen.Recording.2024-08-30.at.9.27.37.AM.mov
  1. Very minor nit but in the category icon button row, where it gets cut and you have to scroll to see more categories, can we remove the white space before the line so that the icon button is visible until the line on both sides? Please refer to the image added above for a visual.

@enahum
Copy link
Contributor

enahum commented Aug 30, 2024

@abhijit-singh just for you to know, accomplishing your animation is extremely complex, specially has you cannot place anything on top of the keyboard area while this one is opened unless a heavy customization goes into work here, I have share some info about that with @Rajat-Dabade but I just don't see it,in fact not even the native keyboard does it. What you see in other apps that take use of that area is the customization that I'm talking about. And you can see how when that is the case, the keyboard area is not dismissable with a gesture

@enahum
Copy link
Contributor

enahum commented Aug 30, 2024

So I just noticed in other apps that the element doing the in/out transition is the keyboard and not the element being shown.

This perhaps would allow us to keep the input area in a specific position in the screen while rendering the emoji picker below it, that would be interesting to see.

One major concern about this is how does it look on a Tablet where the screen is split with lhs and the channel

@abhijit-singh
Copy link

Thanks @enahum, transitioning the keyboard would work equally well from my perspective. Just want to avoid the jumping around of content.

I'll update the recommendation in my previous comment. And here's how it can look for Tablet but no idea if this is technically complex -

Screen.Recording.2024-08-30.at.9.25.11.AM.mov

@Rajat-Dabade Rajat-Dabade added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Sep 2, 2024
@abhijit-singh abhijit-singh added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Sep 9, 2024
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

Great work! Some comments to address though

Comment on lines -57 to -59
const fileActionTestID = `${testID}.file_action`;
const imageActionTestID = `${testID}.image_action`;
const cameraActionTestID = `${testID}.camera_action`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably breaking some e2e tests. Verify with @yasserfaraazkhan and get them fixed, or disabled them so we can fix them later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yasserfaraazkhan Can you please help regarding this?

app/screens/attachment_options/index.tsx Outdated Show resolved Hide resolved
@Rajat-Dabade Rajat-Dabade added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Sep 13, 2024
@yasserfaraazkhan yasserfaraazkhan added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build App for Android Build the mobile app for Android labels Sep 27, 2024
@abhijit-singh
Copy link

@Rajat-Dabade I discussed the transition between the emoji picker and the keyboard with the design team. The feedback there was that the current transition does look jarring because of the center channel content moving around when switching between the two things.

Based on that, here's a simplified request — Can we figure out a way so that the center channel content doesn't jump around? Any other transition animation like sliding is secondary and can be ignored to start with. The center channel content should stay in the same place since we're already ensuring that the emoji picker is of the same size as the keyboard.

Let me know what you think. Happy to discuss in more detail and answer any questions you may have.

@enahum
Copy link
Contributor

enahum commented Sep 27, 2024

Based on that, here's a simplified request — Can we figure out a way so that the center channel content doesn't jump around? Any other transition animation like sliding is secondary and can be ignored to start with. The center channel content should stay in the same place since we're already ensuring that the emoji picker is of the same size as the keyboard.

Let me know what you think. Happy to discuss in more detail and answer any questions you may have.

Looks very very very challenging as the center channel is controlled by the keyboard appearing/disappearing but the keyboard and the emoji picker in the keyboard are not, so as one closes and the other one opens, you get the center channel moving.

To accomplish the above, the current strategy won't work in any way

@Rajat-Dabade Rajat-Dabade added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Sep 28, 2024
@Rajat-Dabade
Copy link
Contributor Author

@abhijit-singh I agree with @enahum, I tried to overlap the content over the keyboard but it didn't work. The keyboard should close before and then the emoji picker will be opened, which will cause the centre channel to move.

since we're already ensuring that the emoji picker is of the same size as the keyboard

No, we aren't. The reason is that if the emoji picker is opened before the keyboard, we don't have the information about the keyboard's height until it is actually opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core commiter 2: UX Review Requires review by a UX Designer 3: QA Review Requires review by a QA tester Build Apps for PR Build the mobile app for iOS and Android to test release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants