-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Feature to highlight active emoji #35140
Feature to highlight active emoji #35140
Conversation
…x/33679-highlight-active-emoji
…x/33679-highlight-active-emoji
@wildan-m when the selected emoji is in |
@getusha That might be the desired behavior, so the user will know that those emojis are the same and don't need to re-select them if they want to change. Are you suggesting that we avoid highlighting frequently used emojis when they are active? |
That makes sense, we can keep it. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-02-05.at.11.03.06.PM.movAndroid: mWeb ChromeScreen.Recording.2024-02-05.at.11.00.49.PM.moviOS: NativeScreen.Recording.2024-02-05.at.10.56.28.PM.moviOS: mWeb SafariScreen.Recording.2024-02-05.at.10.51.04.PM.movMacOS: Chrome / SafariScreen.Recording.2024-02-04.at.6.47.29.PM.movMacOS: DesktopScreen.Recording.2024-02-05.at.11.09.25.PM.mov |
@Expensify/design should we highlight the selected emoji regardless of the skin tone or it is better to highlight the specific emoji that has the initial skin tone? Screen.Recording.2024-02-04.at.6.52.58.PM.mov |
Co-authored-by: Getabalew Tesfaye <75031127+getusha@users.noreply.github.com>
I think we should highlight the emoji itself not the skin tone so what you have in the video there looks good to me. Danny and Shawn might have other thoughts though |
Actually, when you change the skintone in the emoji picker, I'd expect the underlying emoji to change as well. I'm assuming in the video it doesn't stop until you re-select the emoji, but I'm not sure that's ideal. Again keen on @Expensify/design thoughts |
I honestly don't have strong feelings—personally I don't really even see the need to highlight the active emoji 😬. Happy with whatever the team decides here. |
I think I agree with @dannymcclain here, what's the goal with highlighting the current active emoji? Do we even need it? For the other stuff though, I agree with @dubielzyk-expensify here:
|
I raised that here https://expensify.slack.com/archives/C01GTK53T8Q/p1706117484845689, and we landed to highlight the selected emoji inside the Emojipicker. I agree with @dubielzyk-expensify's input too. |
I also agree with @dubielzyk-expensify's input 👍 |
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, just minor comment.
Co-authored-by: Getabalew Tesfaye <75031127+getusha@users.noreply.github.com>
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.
🟢
✋ 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/hayata-suenaga in version: 1.4.38-0 🚀
|
🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.4.38-0 🚀
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.38-6 🚀
|
@getusha, @hayata-suenaga
Details
Add a feature to highlight the active emoji. It is currently being used by the emojiPickerButton.
Fixed Issues
$ #33679
PROPOSAL: #33679 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.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
Screen.Recording.2024-01-25.at.17.08.26.mp4
Android: mWeb Chrome
Screen.Recording.2024-01-25.at.17.11.41.mp4
iOS: Native
Screen.Recording.2024-01-25.at.16.59.06.mp4
iOS: mWeb Safari
Screen.Recording.2024-01-25.at.17.01.00.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-01-25.at.17.02.42.mp4
MacOS: Desktop
Screen.Recording.2024-01-25.at.17.10.55.mp4