-
-
Notifications
You must be signed in to change notification settings - Fork 205
feat: added virualization and added headless search hook #439
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
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.
2. **Network and preloading**
When we don't preload all categories, we also don't preload all emoji images. I just tested the change with a throttled network, and this is what I saw when scrolling down:
This is very unexpected, and we should account for that when supporting virtuzlization. Emojis should still preload.
3. **Keyboard navigation broke**
Keyboard navigation of emojis scrolls the picker up and down above and below the viewport. With virtualization added, it doesn't work anymore for scrolling up, but instead, it exists up to the category list.
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? |
|
@ealush please do take a look at this whenever possible ! |
|
Hey @ealush can you please take a look at this whenever possible |
|
Hey @ealush can you please take a look at this whenever possible |
|
This would be really cool to see make its way into master. I'll have to try out the |
3a2cd81 to
c6203d6
Compare
e1e9bc5 to
76f2b14
Compare



This PR contains the following
1.
useEmojiSearchwhere it will allow to search on top of all the emojisSept.9.Screen.Recording.mov
2.
virualizationwhere it will only render the category which is visible on the DOM#299
Sept.9.Screen.Recording.1.mov