-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Emojis larger in other contexts than just single character messages #40692
Emojis larger in other contexts than just single character messages #40692
Conversation
a121cc6
to
40e2f83
Compare
bdc28a6
to
09116ba
Compare
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
src/libs/EmojiUtils.ts
Outdated
*/ | ||
|
||
// Surrogate pairs (combined emojis) | ||
const highSurrogateStart = 0xd800; |
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.
Let's use SCREAMING_SNAKE_CASE
for all these constants
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.
sure! updated
src/libs/EmojiUtils.ts
Outdated
const variationModifierStart = 0xfe00; // Request text presentation of emoji | ||
const variationModifierEnd = 0xfe0f; // Indicate that the character should be displayed as an emoji | ||
|
||
const codePointFromSurrogatePair = (pair: string) => { |
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.
let's use the function
keyword instead of arrow functions, just because it's the more common pattern in this codebase
src/hooks/useMarkdownStyle.ts
Outdated
import FontUtils from '@styles/utils/FontUtils'; | ||
import variables from '@styles/variables'; | ||
import useTheme from './useTheme'; | ||
|
||
function useMarkdownStyle(message: string | null = null): MarkdownStyle { | ||
function useMarkdownStyle(containsEmojisOnly?: boolean): MarkdownStyle { |
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.
please stay with containsOnlyEmojis
or textContainsOnlyEmojis
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.
fair point, changed to inputContainsOnlyEmojis
src/libs/EmojiUtils.ts
Outdated
return 2; // Variations and non-BMP characters | ||
} | ||
|
||
function splitTextWithEmojis(text: string): string[] { |
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.
I think we could vastly simplify this code if we use a combination of regex + this logic @roryabraham designed in the past.
First, we need to update CONST.REGEX.EMOJIS
to include the new d
flag which I've implemented in the past for this issue and is available now on Hermes engine:
diff --git a/src/CONST.ts b/src/CONST.ts
index f0139d82e6..ebe0ab7549 100755
--- a/src/CONST.ts
+++ b/src/CONST.ts
@@ -1760,8 +1760,8 @@ const CONST = {
// eslint-disable-next-line max-len, no-misleading-character-class
EMOJI: /[\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3/gu,
- // eslint-disable-next-line max-len, no-misleading-character-class
- EMOJIS: /[\p{Extended_Pictographic}](\u200D[\p{Extended_Pictographic}]|[\u{1F3FB}-\u{1F3FF}]|[\u{E0020}-\u{E007F}]|\uFE0F|\u20E3)*|[\u{1F1E6}-\u{1F1FF}]{2}|[#*0-9]\uFE0F?\u20E3/gu,
+ // eslint-disable-next-line max-len, no-misleading-character-class, no-empty-character-class
+ EMOJIS: /[\p{Extended_Pictographic}](\u200D[\p{Extended_Pictographic}]|[\u{1F3FB}-\u{1F3FF}]|[\u{E0020}-\u{E007F}]|\uFE0F|\u20E3)*|[\u{1F1E6}-\u{1F1FF}]{2}|[#*0-9]\uFE0F?\u20E3/dgu,
// eslint-disable-next-line max-len, no-misleading-character-class
EMOJI_SKIN_TONES: /[\u{1f3fb}-\u{1f3ff}]/gu,
Then, let's implement the splitTextWithEmojis
function which will use the regex and indices feature and is based on Rory's initial idea:
type TextWithEmoji = {
text: string;
isEmoji: boolean;
};
function splitTextWithEmojis(text: string): TextWithEmoji[] {
if (!text) {
return [];
}
// The regex needs to be cloned because `exec()` is a stateful operation and maintains the state inside
// the regex variable itself, so we must have a independent instance for each function's call.
const emojisRegex = new RegExp(CONST.REGEX.EMOJIS, CONST.REGEX.EMOJIS.flags);
const splitText: TextWithEmoji[] = [];
let regexResult: RegExpExecArray | null;
let lastMatchIndexEnd = 0;
do {
regexResult = emojisRegex.exec(text);
if (regexResult?.indices) {
const matchIndexStart = regexResult.indices[0][0];
const matchIndexEnd = regexResult.indices[0][1];
if (matchIndexStart > lastMatchIndexEnd) {
splitText.push({
text: text.slice(lastMatchIndexEnd, matchIndexStart),
isEmoji: false,
});
}
splitText.push({
text: text.slice(matchIndexStart, matchIndexEnd),
isEmoji: true,
});
lastMatchIndexEnd = matchIndexEnd;
}
} while (regexResult !== null);
if (lastMatchIndexEnd < text.length) {
splitText.push({
text: text.slice(lastMatchIndexEnd, text.length),
isEmoji: false,
});
}
return splitText;
}
You'll notice some TS errors in the function regarding indices
not being existent. We'll need to add declarations for ES2022 RegExp in tsconfig.json
file:
diff --git a/tsconfig.json b/tsconfig.json
index 7f7b7479f4..ca3cdfa4fa 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -20,7 +20,8 @@
"es2022.object",
"es2022.string",
"ES2021.Intl",
- "ES2023.Array"
+ "ES2023.Array",
+ "ES2022.RegExp"
],
"allowJs": true,
"checkJs": false,
Let's test this function now! If I pass this string Hello world 🙂🙂🙂 ! 😶🌫️😶🌫️😶🌫️ test 😶🌫️ 😶🌫️ test2 👍👍🏿
which has some normal and surrogate emojis, the output will be:
[
{"isEmoji":false,"text":"Hello world "},
{"isEmoji":true,"text":"🙂"},
{"isEmoji":true,"text":"🙂"},
{"isEmoji":true,"text":"🙂"},
{"isEmoji":false,"text":" ! "},
{"isEmoji":true,"text":"😶🌫️"},
{"isEmoji":true,"text":"😶🌫️"},
{"isEmoji":true,"text":"😶🌫️"},
{"isEmoji":false,"text":" test "},
{"isEmoji":true,"text":"😶🌫️"},
{"isEmoji":false,"text":" "},
{"isEmoji":true,"text":"😶🌫️"},
{"isEmoji":false,"text":" test2 "},
{"isEmoji":true,"text":"👍"},
{"isEmoji":true,"text":"👍🏿"}
]
You can adjust the function to just return an array withe the separated tokens if you want, could you give it a try and see if it suits your needs in the PR?
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.
Update: we are still testing the regex solution
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.
@kbieganowski I've updated my previous solution:
- Fixed
splitTextWithEmojis
function to always create a separate regex instance on each call, thus preventing the issue we were discussing internally. - Simplified solution to the TS errors regarding
indices
.
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.
@kbieganowski After internal discussions, we noticed that having the g
(global) flag on CONST.REGEX.EMOJIS
can result in hard-to-detect bugs throughout the App due to the stateful nature of the regex when it has the g
flag. For example, repeatedly calls to test()
with same input will sometimes return different results because the regex is being used globally.
So, I would suggest to remove the g
flag from CONST.REGEX.EMOJIS
:
// eslint-disable-next-line max-len, no-misleading-character-class, no-empty-character-class
EMOJIS: /[\p{Extended_Pictographic}](\u200D[\p{Extended_Pictographic}]|[\u{1F3FB}-\u{1F3FF}]|[\u{E0020}-\u{E007F}]|\uFE0F|\u20E3)*|[\u{1F1E6}-\u{1F1FF}]{2}|[#*0-9]\uFE0F?\u20E3/du,
And then adding it when cloning the regex inside splitTextWithEmojis
:
// We clone the regex and add the `g` (global) flag in order to `exec()` be stateful and work properly.
// Adding the flag directly to CONST.ts can result in bugs throughout the App due to the stateful nature
// of the regex when it has the `g` flag.
const emojisRegex = new RegExp(CONST.REGEX.EMOJIS, CONST.REGEX.EMOJIS.flags.concat('g'));
This change will require testing in all areas of the app that uses CONST.REGEX.EMOJIS
since we removed the g
flag now.
I would advice doing the same for CONST.REGEX.EMOJI
and CONST.REGEX.EMOJI_SKIN_TONES
, that could be done in a follow-up PR to avoid regressions on this one.
bd26c68
to
797cfeb
Compare
5cec5d3
to
1508719
Compare
Hey @fabioh8010, I've validated other usages of the regex and everything seems to work as expected. Ready for a re-review |
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.
Few comments, also please make sure all places where CONST.REGEX.EMOJIS
is used (src/components/InlineCodeBlock/WrappedText.tsx
, src/libs/EmojiUtils.ts
, src/libs/ValidationUtils.ts
) are properly handled after removing the g
flag.
@@ -65,7 +67,7 @@ function Composer( | |||
}, [shouldClear, onClear]); | |||
|
|||
const maxHeightStyle = useMemo(() => StyleUtils.getComposerMaxHeightStyle(maxLines, isComposerFullSize), [StyleUtils, isComposerFullSize, maxLines]); | |||
const composerStyle = useMemo(() => StyleSheet.flatten(style), [style]); | |||
const composerStyle = useMemo(() => StyleSheet.flatten([style, textContainsOnlyEmojis ? {lineHeight: 32} : null]), [style, textContainsOnlyEmojis]); |
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.
const composerStyle = useMemo(() => StyleSheet.flatten([style, textContainsOnlyEmojis ? {lineHeight: 32} : null]), [style, textContainsOnlyEmojis]); | |
const composerStyle = useMemo(() => StyleSheet.flatten([style, textContainsOnlyEmojis ? {lineHeight: variables.lineHeightXXXLarge} : null]), [style, textContainsOnlyEmojis]); |
Maybe did you want to use lineHeightXXXLarge
(or perhaps a different variable)?
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.
I went with new variables rather than using existing ones because this styles are strictly related to composer/input and I didn't want potential changes to the UI in future to break composer or inputs as well but no more magic numbers
@@ -380,6 +380,7 @@ function BaseTextInput( | |||
// Add disabled color theme when field is not editable. | |||
inputProps.disabled && styles.textInputDisabled, | |||
styles.pointerEventsAuto, | |||
isMarkdownEnabled ? {lineHeight: 18} : null, |
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.
isMarkdownEnabled ? {lineHeight: 18} : null, | |
isMarkdownEnabled ? {lineHeight: variables.lineHeightLarge} : null, |
makes sense (or perhaps a different variable)?
src/hooks/useMarkdownStyle.ts
Outdated
@@ -21,7 +19,7 @@ function useMarkdownStyle(message: string | null = null): MarkdownStyle { | |||
fontSize: variables.fontSizeLarge, | |||
}, | |||
emoji: { | |||
fontSize: emojiFontSize, | |||
fontSize: inputContainsOnlyEmojis ? 27 : 19, |
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.
Same, shouldn't we use already defined variables for those values, or create new ones if they are new?
isEdited?: boolean; | ||
emojisOnly?: boolean; | ||
}; | ||
function TextWithEmojiFragment({message, passedStyles, styleAsDeleted, styleAsMuted, isSmallScreenWidth, isEdited, emojisOnly}: ComponentProps) { |
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.
function TextWithEmojiFragment({message, passedStyles, styleAsDeleted, styleAsMuted, isSmallScreenWidth, isEdited, emojisOnly}: ComponentProps) { | |
function TextWithEmojiFragment({message, passedStyles, styleAsDeleted, styleAsMuted, isSmallScreenWidth, isEdited, emojisOnly}: ComponentProps) { |
import variables from '@styles/variables'; | ||
import CONST from '@src/CONST'; | ||
|
||
type ComponentProps = { |
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.
type ComponentProps = { | |
type TextWithEmojiFragmentProps = { |
Also, please add descriptions for each prop
|
||
type ComponentProps = { | ||
message: string; | ||
passedStyles: StyleProp<TextStyle>; |
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.
passedStyles: StyleProp<TextStyle>; | |
passedStyles?: StyleProp<TextStyle>; |
Style props are optional by convention.
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.
Just one nitpick, amazing work! LGTM 🚀
type TextWithEmojiFragmentProps = { | ||
/** The message to be displayed */ | ||
message: string; | ||
/** Additional styles to add after local styles. */ | ||
passedStyles?: StyleProp<TextStyle>; | ||
/** Should this message fragment be styled as deleted? */ | ||
styleAsDeleted?: boolean; | ||
/** Should this message fragment be styled as muted? */ | ||
styleAsMuted?: boolean; | ||
/** Is message displayed on narrow screen? */ | ||
isSmallScreenWidth?: boolean; | ||
/** Should "(edited)" suffix be rendered? */ | ||
isEdited?: boolean; | ||
/** Does message contain only emojis? */ | ||
emojisOnly?: boolean; | ||
}; |
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.
type TextWithEmojiFragmentProps = { | |
/** The message to be displayed */ | |
message: string; | |
/** Additional styles to add after local styles. */ | |
passedStyles?: StyleProp<TextStyle>; | |
/** Should this message fragment be styled as deleted? */ | |
styleAsDeleted?: boolean; | |
/** Should this message fragment be styled as muted? */ | |
styleAsMuted?: boolean; | |
/** Is message displayed on narrow screen? */ | |
isSmallScreenWidth?: boolean; | |
/** Should "(edited)" suffix be rendered? */ | |
isEdited?: boolean; | |
/** Does message contain only emojis? */ | |
emojisOnly?: boolean; | |
}; | |
type TextWithEmojiFragmentProps = { | |
/** The message to be displayed */ | |
message: string; | |
/** Additional styles to add after local styles. */ | |
passedStyles?: StyleProp<TextStyle>; | |
/** Should this message fragment be styled as deleted? */ | |
styleAsDeleted?: boolean; | |
/** Should this message fragment be styled as muted? */ | |
styleAsMuted?: boolean; | |
/** Is message displayed on narrow screen? */ | |
isSmallScreenWidth?: boolean; | |
/** Should "(edited)" suffix be rendered? */ | |
isEdited?: boolean; | |
/** Does message contain only emojis? */ | |
emojisOnly?: boolean; | |
}; |
Add spaces between the props
9b4a01a
to
d10aeb2
Compare
@parasharrajat 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] |
# Conflicts: # src/pages/home/report/comment/TextCommentFragment.tsx # src/styles/index.ts
@roryabraham I've applied your suggestions, please take another look |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@VickyStash How are we going to handle follow-up fixes? #40692 (comment) |
@parasharrajat I can open the issues for the |
@parasharrajat The follow-up PR for the app is ready, please take a look |
Sound awesome. Please tag me to the new issues. |
@parasharrajat I've created two issues in Expensify/react-native-live-markdown#443 I have some doubts about the input dancing bug cause today, during re-testing, I had an idea of how it could be fixed by updating styles on the app side. If it works, I'll open one more follow-up |
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.0.14-1 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.14-6 🚀
|
Details
Fixed Issues
$ #4114
PROPOSAL: n/a
Tests
Composer:
Report history:
Settings:
Settings
->Profile
and add emoji to your last nameOffline tests
n/a
QA Steps
Same as in
Tests
sectionNote: If more details regarding tests flow are needed, they can be found in description of related issue
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
chrome_chat.mp4
MacOS: Desktop
desktop_chat.mp4