Skip to content

[material-ui][docs] Virtualize the icons list search#41644

Closed
sai6855 wants to merge 11 commits intomui:nextfrom
sai6855:virtualize-icons
Closed

[material-ui][docs] Virtualize the icons list search#41644
sai6855 wants to merge 11 commits intomui:nextfrom
sai6855:virtualize-icons

Conversation

@sai6855
Copy link
Member

@sai6855 sai6855 commented Mar 25, 2024

@sai6855 sai6855 marked this pull request as draft March 25, 2024 16:35
@sai6855 sai6855 added docs Improvements or additions to the documentation. performance scope: icons Changes related to the icons. labels Mar 25, 2024
@mui-bot
Copy link

mui-bot commented Mar 25, 2024

Netlify deploy preview

https://deploy-preview-41644--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 13c5cc5

@sai6855 sai6855 changed the title [docs][Icons] Try virtualizing [docs][Icons] Virtualize icons page Mar 25, 2024
@danilo-leal danilo-leal changed the title [docs][Icons] Virtualize icons page [material-ui][docs] Virtualize the icons list search Mar 25, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 26, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 26, 2024
@sai6855 sai6855 marked this pull request as ready for review March 26, 2024 15:53
@Janpot
Copy link
Member

Janpot commented Mar 26, 2024

To note: We already have react-window and react-virtuoso in our dependencies. Not sure how the core team feels about this, but does it really make sense to roll our own virtualization? (e.g. react-virtuoso already supports virtual responsive grids out of the box, I haven't vetted the lib any further)

Looking at the preview, I also feel like the UX is off. i.e. it's very annoying to scroll and try and reach the content underneath the icons. IMO, the icon list should have a maximum height and scroll.

@sai6855
Copy link
Member Author

sai6855 commented Mar 26, 2024

To note: We already have react-window and react-virtuoso in our dependencies. Not sure how the core team feels about this, but does it really make sense to roll our own virtualization?

Oh wasn't aware of these libraries being used.

Cc @mnajdova @DiegoAndai what do you think,? should I port changes to adopt to mentioned libraries

@DiegoAndai
Copy link
Member

@sai6855 thanks for working on this!

what do you think,? should I port changes to adopt to mentioned libraries

Yes, if possible we should use the existent dependencies.

Regarding UX, I think @danilo-leal might be able to guide us into what the interaction should be.

@danilo-leal
Copy link
Collaborator

I also feel like the UX is off. i.e. it's very annoying to scroll and try and reach the content underneath the icons. IMO, the icon list should have a maximum height and scroll.

Just to clarify: content here being the API section links?

@Janpot
Copy link
Member

Janpot commented Mar 28, 2024

Just to clarify: content here being the API section links?

Yep, and the footer, feedback control, next/previous page link,...

@ZeeshanTamboli
Copy link
Member

@sai6855 I wasn’t aware of this PR. I’ve implemented virtualization using react-virtuoso in #41330 and also improved the scrolling UX. It addresses #41126 (or part of #41126?). I'll close this PR in favor of #41330. Feel free to review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to the documentation. performance scope: icons Changes related to the icons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants