WIP: [MM-12741] Refactor emoji picker #3648
Conversation
|
This issue has been automatically labelled "stale" because it hasn't had recent activity. /cc @jasonblais @hanzei |
|
Hi @cometkim! 👋 Let us know if you have any questions - sounds like this might have been a starter PR for initial review based on a note here? mattermost/mattermost#10346 (comment) |
|
Hey @jasonblais , currently I'm working on this only weekend. so this would be delayed. I will make some progress and mention you. |
|
No worries! Thank you for the heads up 👍 |
cometkim
force-pushed
the
refactor-emoji-picker
branch
3 times, most recently
from
3347585
to
246751f
Compare
|
Awaiting #3866 PR to be merged. |
|
Any update on this? |
|
Hey @HarshaLaxman, thank you for watching at this. I've been very busy at work for couple months, so would resume any contribution later this month. And probably going to go ahead from react-bootstrap related issues first because It blocks the use of react's context API. you can see the context from #3866 and #4185 (comment) I think you can just lazy load the picker component in another PR. |
cometkim
force-pushed
the
refactor-emoji-picker
branch
from
e74085f
to
1091755
Compare
Note:
Removed onMouseOver handler, it would be unnecessary after re-impl
EmojiPicker with react-window.
I liked Enzyme until React 16.8, now I miss testing-library.
cometkim
force-pushed
the
refactor-emoji-picker
branch
from
1091755
to
fedf175
Compare
|
Hi @cometkim, anything we can help with here? |
|
Hi @esethna. Since this PR is very old, it is better to rewrite it from scratch. (Another contributor may take it) Sharing here some of what I've researched before. ApproachesI benchmarked a similar component before starting this. emoji-mart vs slack Emoji Mart takes full advantage of HTML/CSS features, so there are almost no JS re-rendering. the initial mount/unmount cost is bit high, but the DOM structure is that not complicated, so it shows quite good performance with less codes and also accessibility is quite good because it is based on basic HTML elements. Slack seems to have chosen Virtualization for more interaction, and sticky section headers etc. have also been implemented directly using JS. Since the DOM structure is not intuitive, it seems like a hack for the A11y will be necessary, but it could perform better and may be better if it optimizes the re-render more. I suggest using the community version of emoji-mart for maintainability. License issueAnother issue is the fact that the images contained in the emoji data used by Mattermost violate Apple's license terms. It is important to understand that contributing to code that uses it poses a risk to individual contributors. So I want to migrate it to open source emoji-data and use other images/emoji-fonts or just system fonts, but Mattermost's emoji codebase is pretty complicated. It seems better suited for the core team. After resolving issues across the webapp and the redux, it will be easier for other contributors to improve the emoji picker. |
|
Thanks for the details @cometkim, closing this PR. I agree it would make sense to bring this in the core team |
Summary
Ticket Link
GitHub: mattermost/mattermost#10346
Jira: MM-12741
Related Pull Requests
useIntl().