-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
base: main
Are you sure you want to change the base?
[Feat]: Emoji Picker #8157
Conversation
/update-branch |
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.
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
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 @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:
- 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. - Borders above and below the search bar are missing.
- The divider just above the footer containing category icon buttons should be edge to edge.
- There is too much space above and below the category icon button row. It can be reduced to match the design.
- 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.
- 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
- 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.
@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 |
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 |
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 |
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.
Great work! Some comments to address though
app/components/post_draft/custom_emoji_picker/emoji_filtered/emoji_filtered.tsx
Outdated
Show resolved
Hide resolved
app/components/post_draft/custom_emoji_picker/emoji_filtered/emoji_filtered.tsx
Outdated
Show resolved
Hide resolved
app/components/post_draft/custom_emoji_picker/emoji_filtered/emoji_filtered.tsx
Outdated
Show resolved
Hide resolved
app/components/post_draft/custom_emoji_picker/emoji_picker_header/emoji_picker_header.tsx
Outdated
Show resolved
Hide resolved
app/components/post_draft/custom_emoji_picker/emoji_picker_header/emoji_picker_header.tsx
Outdated
Show resolved
Hide resolved
const fileActionTestID = `${testID}.file_action`; | ||
const imageActionTestID = `${testID}.image_action`; | ||
const cameraActionTestID = `${testID}.camera_action`; |
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.
This is probably breaking some e2e tests. Verify with @yasserfaraazkhan and get them fixed, or disabled them so we can fix them later.
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.
@yasserfaraazkhan Can you please help regarding this?
8ec66ea
to
b466d86
Compare
@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. |
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 |
@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.
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. |
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