Add clickable custom emoji support for onboarding task#55222
Add clickable custom emoji support for onboarding task#55222carlosmiceli merged 14 commits intoExpensify:mainfrom
Conversation
dfc6476 to
a61a8d5
Compare
a61a8d5 to
c446d64
Compare
|
Hmm, I like the inclusion of the green plus button image... But I don't know how I feel about it being clickable and actually opening the GC menu. @Expensify/design how do you all feel about that? Was that decision made in a convo somewhere that I missed? |
|
The relevant conversation is here. I have mixed feelings. I don't love that this is kind of a "new" way to make an expense, but at the same time, I understand the need to educate our users. If it were up to me I would probably not do this one though, but others seemed to have felt strongly. |
src/styles/index.ts
Outdated
| justifyContent: 'center', | ||
| }, | ||
|
|
||
| floatingActionEmoji: { |
There was a problem hiding this comment.
cc @Expensify/design - part of me thinks we should just provide an .svg image for this rather than creating a custom element via styles.
@Amoralchik is there a reason why you didn't do this via an svg?
There was a problem hiding this comment.
I not good at SVG, and because floatingActionButton already use two SVG elements, I decided to simply resize it
There was a problem hiding this comment.
The design team will provide the svg. But I think it's implementation would be far simpler, and kind of paves the path for future custom emojis should we want more.
There was a problem hiding this comment.
There was a problem hiding this comment.
@Amoralchik can you try with this image quickly? custom-emoji__global-create.svg.zip
Note that the svg is 20px tall, and the line height of the text surrounding it is also 20px tall. So it should all line up perfectly like this:

There was a problem hiding this comment.
@shawnborton Hello, sorry for the wait. I’ve rewritten the code using the SVG icon you provided. I feel like I might not have fully understood what you meant, so here is the result with 18px and 20px sizes.
There was a problem hiding this comment.
Ah sorry, please use the icon I provided to you at 20px. What I did was make the inner green circle 18px within a 20px bounding box. Let me know if that makes sense.
There was a problem hiding this comment.
@shawnborton Okay, I updated the code using an icon size of 20px. I was wondering if it might be worth increasing the size of the "+" icon. I know it might just be me, but I sometimes find it a bit hard to see it against a green circle.
I mean that this icon currently just replicates the main one, which is probably correct, but I think some people might find it hard to notice the "+" on a phone screen or a very large monitor.
There was a problem hiding this comment.
Let's see what the other designers think, cc @Expensify/design
Personally I think it's fine because it's basically just a scaled down version of the existing global create, so it basically has the same proportions that the normal button has. I can see wanting to correct it optically a little bit, but it doesn't bother me personally.
There was a problem hiding this comment.
I can see wanting to correct it optically a little bit, but it doesn't bother me personally.
This is pretty much exactly how I feel. I think it's probably fine as-is, so maybe we wait and see if we receive any feedback about it before changing it? Seems like a simple change to replace the SVG in the future if we think we need to.
Ok thank you for the context! I guess I was involved in that convo and just didn't realize we were planning to add that functionality. I would prefer it to not have it, but I won't stand in the way. |
|
Just to confirm, this is kind of two totally different features at work:
These are completely distinct, unrelated features, that happen to work well together (ie, if you hyperlink |
@quinthar is there something happening in this PR that caused you to make that comment? Just want to make sure I am understanding your concern correctly. |
|
The FAB does not have a screen. So, based on the Slack discussion, creating a link solely to open the FAB is odd. So, we are using custom renderers to open the popover as well when there is no screen associated with the popover. When we want to simulate a button click to open a popover that does not have a screen (like the FAB in this case) for an emoji, we will use a default press attribute in the custom-emoji to make the emoji clickable.
For emojis linking to routes that exist, this is exactly how we do it: [<custom-emoji emoji="emoji-name" />](URL)However, when there is no URL or screen for the action triggered by clicking the emoji, we use: <custom-emoji emoji="emoji-name" pressablewithdefault />When the
So, this press effect does not appear everywhere the emoji is present but only at selective places where we pass the attribute. |
f508b3b to
b1e3dbc
Compare
|
Updated |
|
Updated |
|
Going to run a test build on this one. |
|
🚧 @shawnborton has triggered a test build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
Sorry, I think I must have used your earlier PR when iOS worked. I checked and tried different ways to make this work on iOS but could not. On iOS, popovers do not seem to work for screens out of view in the same way as Android. We gotta use your earlier solution of passing emoji prop to the FABPopover and FAB when we want the emoji to be interactive and open the FAB. Please remove the context based changes and let us use your earlier logic in |
src/CONST.ts
Outdated
| 'Here’s how to submit an expense:\n' + | ||
| '\n' + | ||
| '1. Click the green *+* button.\n' + | ||
| '1. Press the <custom-emoji emoji="action-menu-icon" pressablewithdefaultaction /> button.\n' + |
There was a problem hiding this comment.
If pressablewithdefaultaction is removed, it should render a non-interactive emoji. It is crashing for me for this case on web.
Screen.Recording.2025-01-29.at.5.29.49.PM.mov
There was a problem hiding this comment.
Thanks for this, I think I know why it happened
Additionally sorry, I did not make full testing, I pushed to pull and check the IOS on the laptop
… rendering emoji content
|
Updated return earlier logic All video rerecorded |
src/libs/ReportUtils.ts
Outdated
| description?: string, | ||
| policyID: string = CONST.POLICY.OWNER_EMAIL_FAKE, | ||
| notificationPreference: NotificationPreference = CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN, | ||
| shouldEscapeText?: boolean, |
There was a problem hiding this comment.
| shouldEscapeText?: boolean, | |
| shouldEscapeText = true, |
parse of ExpensiMark uses true as default for shouldEscapeText. So, let us default this to true and override it only when required.
| const popoverMenu = ( | ||
| <PopoverMenu | ||
| onClose={hideCreateMenu} | ||
| isVisible={isCreateMenuActive} |
There was a problem hiding this comment.
| isVisible={isCreateMenuActive} | |
| isVisible={isCreateMenuActive && (!shouldUseNarrowLayout || isFocused)} |
Since we are using this component directly in task description too this change is not required. Let us keep it as it was.
| import variables from '@styles/variables'; | ||
|
|
||
| // eslint-disable-next-line rulesdir/no-inline-named-export | ||
| export const emojiMap: Record<string, FC<SvgProps>> = { |
There was a problem hiding this comment.
Let us minimize the lint overrides
| export const emojiMap: Record<string, FC<SvgProps>> = { | |
| const emojiMap: Record<string, FC<SvgProps>> = { |
| // eslint-disable-next-line rulesdir/no-inline-named-export | ||
| export const emojiMap: Record<string, FC<SvgProps>> = { | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| 'action-menu-icon': GlobalCreateIcon, |
There was a problem hiding this comment.
| 'action-menu-icon': GlobalCreateIcon, | |
| 'actionmenuicon': GlobalCreateIcon, |
| function CustomEmojiRenderer({tnode}: CustomRendererProps<TText | TPhrasing>) { | ||
| const styles = useThemeStyles(); | ||
| const emojiKey = tnode.attributes.emoji; | ||
| const positionFix = Platform.OS !== 'web' && {height: '5%'}; |
There was a problem hiding this comment.
I do not understand how restricting the height of the View moves its position on iOS. When I give a background color to the View with this styling the View appears very small and the image overflows but somehow it changes position. Can you please explain this? I have a suggestion using margin in styles/index as specified below which completely removes positionFix. Please check this and see if it is preferable.
|
|
| /> | ||
| ); | ||
|
|
||
| const positionFix = Platform.OS !== 'web' && {height: '5%'}; |
There was a problem hiding this comment.
I suggested removing positionFix here too here. This is also not required anymore. It looks fine after removing this in my testing on native apps. Have you found any issue with removing this?
There was a problem hiding this comment.
No, sorry, I just forgot that we have positionFix here as well. I retested and everything good. Thank you for noticing this.
|
Let me know when this is ready for my review :) |
|
Updated, and I think we're ready if @c3024 agrees |
Reviewer Checklist
Screenshots/VideosAndroid: NativefabAndroid.movAndroid: mWeb ChromefabAndroidmWeb.moviOS: NativefabiOS.moviOS: mWeb SafarifabiOSmWeb.movMacOS: Chrome / SafarifabChrome.movfabChromeNarrow.movMacOS: DesktopfabDesktop.mov |
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.0.94-0 🚀
|
|
This caused a couple DeployBlockers that aren't necessarily trivial to fix, so we're going to revert: #56362 |
|
Okay, I looking into it also, I never fall into a situation like this, I can update this PR with fixes, right? |
|
@Amoralchik you'll need to create a new pull request. You can open a new one from your existing branch, including the additional fixes for the bugs that were found in the staging environment |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.94-25 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.94-25 🚀
|






Explanation of Change
Add clickable custom emoji support for onboarding task
Fixed Issues
$ #54643
PROPOSAL: #54643 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)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
2025-01-31.18.09.35.mov
Android: mWeb Chrome
2025-01-31.18.15.52.mov
iOS: Native
2025-01-31.17.55.29.mov
iOS: mWeb Safari
2025-01-31.17.56.57.mov
MacOS: Chrome / Safari
2025-01-31.18.23.25.mov
MacOS: Desktop
2025-01-31.18.21.35.mov