Skip to content

Add font support for emojis #661

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

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

Conversation

c3024
Copy link

@c3024 c3024 commented Apr 16, 2025

Details

We are adding a custom emoji font for custom emoji support using Private User Area Unicodes.

App PR
Hybrid App PR
expensify-common PR

The current PR updates emoji style to support a font.

Related Issues

Expensify/App#54643

Manual Tests

With applying the App and Hybrid App PRs specified above,

  1. Type :global_ and select the :global_create: emoji from the suggestions.
  2. Verify that the global_create emoji (FAB) gets rendered in the composer.
customEmojiAndroid.mov
customEmojiiOS.mov
customEmojiChrome.mov

Linked PRs

App PR

Copy link

github-actions bot commented Apr 16, 2025

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

@c3024
Copy link
Author

c3024 commented Apr 16, 2025

I have read the CLA Document and I hereby sign the CLA

CLABotify added a commit to Expensify/CLA that referenced this pull request Apr 16, 2025
@tomekzaw tomekzaw self-requested a review April 16, 2025 06:29
Copy link
Collaborator

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

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

LGTM, left one comment.

const FONT_FAMILY_EMOJI = Platform.select({
ios: 'Apple Color Emoji',
android: 'Noto Color Emoji',
default: 'Apple Color Emoji',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this will work on web across non-apple devices.

@tomekzaw
Copy link
Collaborator

Also please mention emojiFontFamily in "Styling" section in the README.

tomekzaw
tomekzaw previously approved these changes Apr 16, 2025
@c3024
Copy link
Author

c3024 commented Apr 16, 2025

Web tests are failing sometimes. Not sure why.

There seems to be a problem on web. The emoji appears back in the composer after sending a message with an emoji.

Screen.Recording.2025-04-16.at.8.06.55.PM.mov

I don't understand which changes for web are causing this.

@c3024
Copy link
Author

c3024 commented Apr 16, 2025

Was there ever a problem like this with any other style?

@shubham1206agra
Copy link

@c3024 This problem exists on main too. See Expensify/App#59290 (comment)

@c3024
Copy link
Author

c3024 commented Apr 17, 2025

@shubham1206agra

Thanks. I bashed my head yesterday trying to see how this PR could cause this bug. If I bump up the rnmarkdown version to 0.1.266, the bug happens on App main too.

@c3024
Copy link
Author

c3024 commented Apr 17, 2025

All review suggestions addressed.

@c3024
Copy link
Author

c3024 commented Apr 17, 2025

@Skalakid @tomekzaw

This happens if RNMarkdown is bumped to 0.1.266 on Expensify/App main too. Any PR in the works to fix this?

@Skalakid
Copy link
Collaborator

This PR causes the issue. I think we will revert it and try to fix it in a different way

@VickyStash
Copy link
Contributor

@c3024 When I tried to bump react-native-live-markdown lib in my PR, I've met the problem that the app is crashing on ios if I try to past the text to the composer. Please, double check that it's not happen to you, just in case 🙏

@c3024
Copy link
Author

c3024 commented Apr 18, 2025

@VickyStash , yes, it's crashing on iOS for me too on version 0.1.264.

@VickyStash
Copy link
Contributor

VickyStash commented Apr 18, 2025

This PR causes the issue. I think we will revert it and try to fix it in a different way

@Skalakid Hm, I've just double-checked the conversation here, and it looks like the issue you are referring to existed on the E/App main as well. Though the bump PR wasn't even merged. So I don't think the problem was in my PR.

@Skalakid
Copy link
Collaborator

Skalakid commented Apr 18, 2025

@VickyStash Hmm, I'm not sure about that. I can't reproduce this bug on the latest main. Here I recorded 2 videos: with and without InteractionManager.runAfterInteractions. On both, I'm using the latest Expensify main and Live Markdown version 0.1.244. On the second video, I've added InteractionManager.runAfterInteractions

Without InteractionManager With InteractionManager
without.mov
with.mov

@joekaufmanexpensify
Copy link

@VickyStash @Skalakid could you share an update on the next steps to unblock merging this PR?

@VickyStash
Copy link
Contributor

Hey, I'm not really familiar with this issue/PR, I was just mentioned here cause my PR (which is now reverted) has been causing some issues during testing.
Also, I've found the crash issue on ios during testing of my own PR, so just mentioned it here so it's not lost.
I believe @tomekzaw or @Skalakid should have more context about this one!

@tomekzaw
Copy link
Collaborator

@VickyStash Hey, as for the iOS crash during pasting, this has already been fixed in #668.

@c3024
Copy link
Author

c3024 commented Apr 29, 2025

Oh, can this PR be merged then? I've addressed the review suggestions.

Copy link
Collaborator

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

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

LGTM

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.

6 participants