-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Enlarge emojis in other contexts than just single character messages #47547
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
Conversation
# Conflicts: # src/components/Composer/index.tsx
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@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/components/Composer/index.native.tsx # src/pages/home/report/comment/TextCommentFragment.tsx
| get ALL_EMOJIS() { | ||
| return new RegExp(this.EMOJIS, this.EMOJIS.flags.concat('g')); |
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.
Do we even need this, or could we directly add the g flag in EMOJIS above? As far as I can tell, the only place we use EMOJIS is in EmojiUtils.splitTextWithEmojis, and we also add g flag there.
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.
@francoisl I think the answer to your question is here:
#40692 (comment)
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.
Got it, thanks.
Is there any situation where we'll want to use CONST.REGEX.EMOJIS without the g flag? If not, let's at least add the comment about cloning that's currently in splitTextWithEmojis() in here, to add context to other people that read it and wouldn't know if they need to use EMOJIS or ALL_EMOJIS.
Ideally though, we could move it to a more private scope that's not available outside of this file, so that someone can't accidentally use CONST.REGEX.EMOJIS.
|
@pecanoro @francoisl I've applied your feedback, let me know if you have more questions! |
| return []; | ||
| } | ||
|
|
||
| const doesTextContainEmojis = new RegExp(CONST.REGEX.EMOJIS, CONST.REGEX.EMOJIS.flags.concat('g')).test(text); |
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.
Can we not use CONST.REGEX.ALL_EMOJIS instead here?
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.
Replacing it here with CONST.REGEX.ALL_EMOJIS causing an issue on ios 🤔
So I've started to think maybe it makes sense to replace CONST.REGEX.ALL_EMOJIS with new RegExp(CONST.REGEX.EMOJIS, CONST.REGEX.EMOJIS.flags.concat('g')) everywhere, though it seems to work as expected in other places.
| get ALL_EMOJIS() { | ||
| return new RegExp(this.EMOJIS, this.EMOJIS.flags.concat('g')); |
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.
Got it, thanks.
Is there any situation where we'll want to use CONST.REGEX.EMOJIS without the g flag? If not, let's at least add the comment about cloning that's currently in splitTextWithEmojis() in here, to add context to other people that read it and wouldn't know if they need to use EMOJIS or ALL_EMOJIS.
Ideally though, we could move it to a more private scope that's not available outside of this file, so that someone can't accidentally use CONST.REGEX.EMOJIS.
# Conflicts: # src/libs/EmojiUtils.tsx
|
@parasharrajat @VickyStash Could you both do a quick test again since we have added a few more commits and confirm it's all good? Just to make sure it is not broken |
|
Will do. |
|
NOTE: There is a slightest change in line-height when we click on the single line input with emoji. This only happens when the value starts with normal text and there is an emoji later. 21.11.2024_12.52.17_REC.mp4 |
parasharrajat
left a comment
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.
LGTM.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/francoisl in version: 9.0.66-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.66-8 🚀
|
|
|
||
| emojisWithTextFontSizeAligned: { | ||
| fontSize: variables.fontSizeEmojisWithinText, | ||
| marginVertical: -7, |
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.
@VickyStash @parasharrajat what was this vertical margin fixing? coming from #53451
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.
@getusha It was used to align emojis on iOS. So I think we can change it on android to fix the mentioned issue.
Please, remember to test it with different emojis texts, for example with
A multiine text with a big para to test yo main. Now I am testing So let's test 😀 HEH Now he sdsds let see 🥲 and 🥲 overlapping messages so This looks like wor 🙂 hey majun whats wrong
and make sure there are no overlapping and line spacing is equivalent between lines.
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.
thank you!
| textContainsOnlyEmojis ? styles.onlyEmojisTextLineHeight : {}, | ||
| textContainsOnlyEmojis && hasMultipleLines ? styles.onlyEmojisTextLineHeight : {}, |
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.
@VickyStash any idea why this was added here? It causes #55068
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.
@CyberAndrii Without hasMultipleLines check even one emoji changes the composer height and makes send button be not aligned in the composer.
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.
We decide to remove hasMultipleLines completely. #55438


Details
This is another try to merge the PR related to emojis enlarging, the previous one was reverted.
Fixed Issues
$ #4114
Potentially fixing #14753
PROPOSAL: N/A
Tests
Composer:
Report history:
Settings:
Offline tests
Same as in the Tests section.
QA Steps
Same as in the Tests section.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop