-
-
Notifications
You must be signed in to change notification settings - Fork 181
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
feat: added virualization and added search hook #439
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@ealush can you please help merge this PR |
1 similar comment
@ealush can you please help merge this PR |
@ealush Can you please check this PR and let me know if there's anything that needs to be done |
Hi
Sorry for taking the time with this, I was unable to allocate much time for open source work recently due to work and travel. I will prioritize reviewing this.
…On Wed, 4 Dec 2024 at 6:44 Gowtham T G ***@***.***> wrote:
@ealush <https://github.com/ealush> Can you please check this PR and let
me know if there's anything that needs to be done
—
Reply to this email directly, view it on GitHub
<#439 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACV32P7ZEX24QNJYJD53VM32D2QEZAVCNFSM6AAAAABN3WK4OWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJWGMZDQMBSHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
First of all @GowthamTG thank you for showing interest in contributing to the picker. It must have been challenging to enter a foreign codebase and still make yourself productive. Kudos. Again, sorry for taking the time reviewing this. I was busy with work and travel the past three months and could not spare much time for unplanned OSS work. Now, there are a lot of things to consider here, since these are really two distinct changes that don't necessarily need to be presented to the picker together. I will make both comments here: VirtualizationI am all for that! Very happy to see that you were able to pull that off quite smoothly, but with that, I do see several challenges.
![]() ![]() Search hookAs something that's not a distinctly a picker feature, I do not fully understand the usecase, so I would love to understand
![]() |
Hi @ealush VirtualisationCategory Highlighting : I've restored the category highlighting functionality by implementing a tracking mechanism that determines the currently visible category based on scroll position within the virtualised list. This now matches the behavior seen on the reference implementation. Network and Preloading : I've implemented proper preloading for emoji images even with virtualization. The emojis now preload in the background regardless of their viewport visibility, preventing the blank image issue on slow networks. This maintains the smooth scrolling experience users expect. Keyboard Navigation: Fixed the keyboard navigation to work properly with virtualization. The up/down navigation now correctly scrolls items into view even when they're outside the virtualised viewport. useEmojiSearchThe motivation behind adding a separate search hook is to provide a headless search functionality that many applications need. While the existing useFilter hook works great for the picker's internal filtering, having a dedicated search hook serves a different use case: It allows applications to implement emoji search in their editors/inputs without embedding the full picker It's focused solely on search functionality without picker-specific logic Please do let me know if you would like me to adjust any of these implementations further or provide more details about specific aspects? |
This PR contains the following
1.
useEmojiSearch
where it will allow to search on top of all the emojisSept.9.Screen.Recording.mov
2.
virualization
where it will only render the category which is visible on the DOM#299
Sept.9.Screen.Recording.1.mov