-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Migrate EmojiPickerMenu to FlashList #31479
Migrate EmojiPickerMenu to FlashList #31479
Conversation
@cubuspl42 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] |
Exception: emojipicker-flashlist-migration-ios.mp4 |
4f41024
to
a55a640
Compare
@cubuspl42 thanks for testing, I addressed both of your comments |
hi @cubuspl42, did you have a chance to take another look on this PR? |
@TMisiukiewicz I will; I have a personal situation that affects my capacity. Everything will be back to normal after Friday. Is this urgent or conflict-prone? |
@cubuspl42 no worries, it can wait until you are back! |
hi @cubuspl42, will you find some time to check this PR this week? 🙏 |
@TMisiukiewicz I'll do this right now, thank you for reminding ♡ |
Please also merge |
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.
Left one comment, otherwise things look good!
a55a640
to
62d795e
Compare
|
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.
👍
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.
Also please investigate what can be DRYed up between the web and mobile implementations, creating a Base component if necessary
0a85015
to
b482d91
Compare
@roryabraham thanks for your review - created a Base component to share some parts of it across platforms and also extracted the common functions, where possible. Mind taking a look once again? |
1b1f5de
to
2d96196
Compare
@roryabraham thanks for your eagle-eye reviews! Really appreciate all the stuff you keep findind and your knowledge of the project. I addressed your comments and fixed the flicker on the initial list render I mentioned on Friday |
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.
Thanks for all your work on this. It's much better than it was. I do want us to follow-up with a PR to build support for ArrowLeft
and ArrowRight
in useArrowKeyFocusManager
as that will further reduce platform differences between these files, I believe to the point where everything can be recombined into a single component.
@cubuspl42 there have been a lot of changes here since your last review. Can you please review and test again before I merge this? I see the reviewer checklist is failing, maybe try again with a fresh checklist? |
thanks for your review @roryabraham! Can we create an issue for supporting |
@TMisiukiewicz Would you merge |
@cubuspl42 whoops that's a pretty big number, updated! |
Too big diversion can result in lint issues and other problems, that's why C+ often remind about merging |
Reviewer Checklist
Screenshots/VideosWebflashlist-web.mp4Mobile Web - Chromeflashlist-android-web-compressed.mp4Mobile Web - Safariflashlist-ios-web.mp4DesktopiOSflashlist-ios.mp4Androidflashlist-android-compressed.mp4 |
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.
I re-tested, and things still work cross-platform!
✋ 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: 1.4.17-0 🚀
|
// On windows, flag emojis are not supported | ||
const emojisForOperatingSystem = | ||
getOperatingSystem() === CONST.OS.WINDOWS | ||
? emojis.slice( | ||
0, | ||
emojis.findIndex((emoji) => { | ||
if (!('header' in emoji)) { | ||
return; | ||
} | ||
|
||
return emoji.header && emoji.code === 'flags'; | ||
}), | ||
) | ||
: emojis; |
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.
@TMisiukiewicz This was implemented during the time you have worked on this PR but we have added the support for flag emojis on Windows in this issues and pr #31717
There was this issue raised here #33593, if you could comment on it so I can assign you and you could make a follow up to remove this Windows specific code. thanks!
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.17-8 🚀
|
|
||
if (item.header) { | ||
return ( | ||
<View style={[styles.emojiHeaderContainer, target === 'StickyHeader' ? styles.mh4 : {width: windowWidth}]}> |
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.
Coming from #33709 (comment)
We had a bug here scroll not working over header.
RCA: emoji header is wrapped in an absolute container and overlaps with scroll bar
// Set scrollPaddingTop to consider sticky headers while scrolling | ||
{scrollPaddingTop: isFiltered ? 0 : CONST.EMOJI_PICKER_ITEM_HEIGHT}, | ||
{scrollPaddingTop: isListFiltered ? 0 : CONST.EMOJI_PICKER_ITEM_HEIGHT}, |
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 line is applied at the wrong place, which created #36883
Details
Migrating EmojiPickerMenu to FlashList for both Web and Mobile
Fixed Issues
$ #30911
PROPOSAL:
Tests
Offline tests
n/a
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(themeColors.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
ANDROID.mov
Android: mWeb Chrome
ANDROID_WEB.mov
iOS: Native
IOS.mp4
iOS: mWeb Safari
IOS_WEB.mp4
MacOS: Chrome / Safari
WEB.mov
MacOS: Desktop
DESKTOP.mov