-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add Emoji Category Picker #14532
Add Emoji Category Picker #14532
Conversation
@stitesExpensify Here as we are trying to scroll to the category header, we need to provide diff --git a/src/components/EmojiPicker/EmojiPickerMenu/index.native.js b/src/components/EmojiPicker/EmojiPickerMenu/index.native.js
index bf9ac71bcf..cc4e46312a 100644
--- a/src/components/EmojiPicker/EmojiPickerMenu/index.native.js
+++ b/src/components/EmojiPicker/EmojiPickerMenu/index.native.js
@@ -111,6 +111,10 @@ class EmojiPickerMenu extends Component {
this.emojiList.scrollToOffset({offset: testoffset, animated: false});
}
+ getItemLayout(data, index) {
+ return {length: CONST.EMOJI_PICKER_ITEM_HEIGHT, offset: CONST.EMOJI_PICKER_ITEM_HEIGHT * index, index};
+ }
+
/**
* Given an emoji item object, render a component based on its type.
* Items with the code "SPACER" return nothing and are used to fill rows up to 8
@@ -167,6 +171,7 @@ class EmojiPickerMenu extends Component {
this.isMobileLandscape() && styles.emojiPickerListLandscape,
]}
stickyHeaderIndices={this.unfilteredHeaderIndices}
+ getItemLayout={this.getItemLayout}
/> |
Ah, I see, thank you. For some reason I thought it was added to both. Going back and looking, I see that we said it was only happening on web/desktop which is interesting... I'm glad the solution works across the board though |
That definitely helped, but there is still a bit of lag (could just be because of the emulator?). I think that adding the animation makes it less noticible though: No animation: |
Why would it only happen on the emulators and not on web/desktop then? |
@stitesExpensify reanimated library is updated now. please pull from latest |
What does that mean @aimane-chnaif ? Also, I updated native to reanimated, but just switched the web version to |
I mean version bump. #14946 |
Oh, are you saying because it was updated, we need to test again with the new version? |
Exactly |
Updated, and it still seems to be performing correctly! |
This is now ready for 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.
LGTM!
Screenshots/Videos
Web
Web.mov
Mobile Web - Chrome
Android.Web.mov
Mobile Web - Safari
iOS.Web.mov
Desktop
Desktop.mov
iOS
iOS.mov
Android
Android.mov
Reviewing now |
this.emojiList.scrollToOffset({offset: calculatedOffset, animated: false}); | ||
const node = findNodeHandle(this.emojiList); | ||
runOnUI(() => { | ||
'worklet'; |
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 add comment what's it for? Similar to
App/src/components/AvatarCropModal/AvatarCropModal.js
Lines 288 to 291 in 8dffdf8
// We are using the worklet directive here and running on the UI thread to ensure the Reanimated | |
// shared values are updated synchronously, as they update asynchronously on the JS thread. | |
'worklet'; |
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.
Hey @aimane-chnaif IMO I don't think the comment will add value there. Will merge the P.R. here : )
Some feedback: (not new)
feedback1.mp4
feedback2.mp4
msafari.MP4
msafari2.MP4Except these issues, the main feature tests well on all platforms. |
✋ 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/PauloGasparSv in version: 1.2.71-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.2.71-1 🚀
|
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/239100
Tests
All Platforms
Web / Desktop
Offline tests
n/a functionality is not affected by online/offline
QA Steps
All Platforms
Web / Desktop
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
2023-02-02_11-32-27.mp4
Mobile Web - Chrome
2023-02-02_11-51-46.mp4
Mobile Web - Safari
2023-02-02_11-50-22.mp4
Desktop
2023-02-02_11-53-39.mp4
iOS
2023-02-02_11-49-27.mp4
Android
2023-02-10_13-12-09.mp4