Clickable emoji for expense task #56602
Conversation
@roryabraham I can't say for sure if it is similar to issues you mentioned. In my opinion it is better to have a conversation here. |
src/components/HTMLEngineProvider/HTMLRenderers/CustomEmojiRenderer.tsx
Outdated
Show resolved
Hide resolved
…derer.tsx Co-authored-by: c3024 <[email protected]>
Co-authored-by: c3024 <[email protected]>
|
Open for review |
|
Do we have updated screenshots to review? |
|
Tried this locally but couldn't get Concierge to send the onboarding message. It usually triggers, but unsure if it's related to this PR or if dev is just being weird. On iOS native the positioning looks great: But on macOS and Android it seems to sit quite a bit higher: Any reason for this? |
|
🚧 @roryabraham has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Updated |
|
Still seeing it pretty high on web |
|
@dubielzyk-expensify Hi, |
| if (platform !== CONST.PLATFORM.WEB) { | ||
| return; | ||
| } | ||
| document.removeEventListener('dragover', hidePopoverOnDragOver); |
There was a problem hiding this comment.
Is this still necessary? We are not adding this event listener for removing it.
There was a problem hiding this comment.
This code is initially from BottomTabBarFloatingActionButton
| import React, {createContext, useCallback, useMemo, useRef, useState} from 'react'; | ||
| import type {View} from 'react-native'; | ||
| import getPlatform from '@libs/getPlatform'; | ||
| import type FloatingActionButtonPopoverMenuRef from '@pages/home/sidebar/BottomTabBarFloatingActionButton/types'; |
There was a problem hiding this comment.
We can move this type to this file and remove index.native.ts and types of BottomTabBarFloatingActionButton because we are not using BottomTabBarFloatingActionButton anymore!
| <PressableWithoutFeedback | ||
| onPress={() => setIsCreateMenuActive(!isFabActionActive)} | ||
| onLongPress={() => {}} | ||
| style={{verticalAlign: 'bottom', userSelect: 'none'}} |
There was a problem hiding this comment.
| style={{verticalAlign: 'bottom', userSelect: 'none'}} | |
| style={[styles.verticalAlignBotton, styles.userSelectNone]} |
| @@ -223,7 +224,9 @@ function NavigationRoot({authenticated, lastVisitedPath, initialUrl, onReady, sh | |||
| enabled: false, | |||
| }} | |||
| > | |||
| <AppNavigator authenticated={authenticated} /> | |||
| <FABPopoverProvider> | |||
There was a problem hiding this comment.
Any reason for separately wrapping the Provider here unlike adding in Expensify.tsx like we do with other Providers?
There was a problem hiding this comment.
We need NavigationContainer for FloatingActionButtonPopover (aka FABPopoverContext) if we place it in Expensify.tsx we fail with this error:
Couldn't find a navigation object. Is your component inside NavigationContainer?
| onPress={toggleCreateMenu} | ||
| /> | ||
| </View> | ||
| </> |
There was a problem hiding this comment.
Now that FAB is moved to another component we can rename this component to FloatingActionButtonPopover.
|
Resolve merge conflict |
|
The new screenshots look good from my end 👍 |
|
Same! Going to fire up a quick test build though because I'm curious if browser zooming has any impact on this one, but otherwise I think we're good to go. |
|
🚧 @shawnborton has triggered a test hybrid app 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! 🧪🧪 |
|
Cool, looks good to me after a quick test. Time for final review @c3024 ! |
Screenshots/VideosAndroid: NativeScreenrecorder-2025-03-04-20-32-51-747.mp4Android: mWeb ChromeScreenrecorder-2025-03-04-20-35-40-117.mp4iOS: NativeemojiiOS-compressed.mp4iOS: mWeb SafariScreenRecording_03-04-2025.20-47-44_1.MP4MacOS: Chrome / SafariemojiChrome.movMacOS: Desktop |
|
Everything tests well. On Android, the emoji layout does not follow text the same way it does on iOS. Different devices have different scales, making it impossible to place emojis accurately at extreme zoom-ins and zoom-outs. On my Redmi Note 11 Pro+, there’s some misalignment at extreme zooms, but it’s the same for regular emojis as well. It looks fine on the emulator for all scales. Since Android alignment cannot be fixed across all zoom levels, I think we can ship this! |
|
✋ 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/roryabraham in version: 9.1.9-0 🚀
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.1.9-0 🚀
|
tgolen
changed the title
[CP Staging] Revert "Merge pull request #56602 from Amoralchik/clickable-emoji-for…
* main: (192 commits) Update Mobile-Expensify submodule version to 9.1.9-6 Update version to 9.1.9-6 reset the submodule commit to fix crash Revert "fix: Message Previews Always Include Sender Name" Update Mobile-Expensify submodule version to 9.1.9-5 Update version to 9.1.9-5 Update Mobile-Expensify submodule version to 9.1.9-4 Update version to 9.1.9-4 Revert "Merge pull request #56602 from Amoralchik/clickable-emoji-for-exoense-task" Update Require-tags-and-categories-for-expenses.md Update Require-tags-and-categories-for-expenses.md bump Mobile-Expensify submodule remove unused code Update Require-tags-and-categories-for-expenses.md Add domain function back to help doc Update Mobile-Expensify submodule version to 9.1.9-3 Update version to 9.1.9-3 avoid deleted employee in exporter list update comment Update redirects.csv ...
…ickable-emoji-for-exoense-task"" This reverts commit 65d675b.
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.1.9-8 🚀
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.1.10-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.1.10-6 🚀
|
Explanation of Change
Fixed Issues
$ #54643
PROPOSAL: #54643 (comment)
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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-02-25.20.44.49.mov
Android: mWeb Chrome
2025-02-25.20.49.49.mov
iOS: Native
2025-02-24.22.17.06.mov
iOS: mWeb Safari
2025-02-25.20.52.29.mov
MacOS: Chrome / Safari
2025-02-25.20.41.03.mov
MacOS: Desktop
2025-02-25.20.38.48.mov