search

twentyhq / twenty

Building a modern alternative to Salesforce, powered by the community.
https://twenty.com
Other
20.22k stars 2.24k forks source link

Table of icons in create new view is not scrollable #6864

Open BOHEUS opened 2 months ago

BOHEUS commented 2 months ago

Scenario:

  1. Log in
  2. Go to e.g. Companies
  3. Click on number of rows, then "Add view" button"
  4. Try to scroll table of available icons image

Actual: Table of icons in create new view is not scrollable

Expected: Table of icons in create new view should be scrollable, icons must be lazy loaded to have the same user experience, despite over available 4000+ icons, only 25 must be visible at the time, search placeholder must reflect amount of icons (change to "Search 4000+ icons")

Faisal-imtiyaz123 commented 2 months ago

@BOHEUS It's because there is max-height set . This is related to #6105 . That PR will also solve it.

FelixMalfait commented 4 weeks ago

Closing in favor of #6105

harshrajeevsingh commented 3 weeks ago

Hey @BOHEUS @FelixMalfait, this issue should be reconsidered again. It's happening not because of max-height, as mentioned by Faizal. The max-height property is already set as none. It's happening because we are rendering a fixed number of icons, i.e., 25.

If you want more clarification, please look at the screenshot. I increased the number of icons to 42. Screenshot from 2024-10-17 20-26-45

FelixMalfait commented 3 weeks ago

@harshrajeevsingh good point! But there are so many icons / I'm afraid it might be complex to build something nice here. They'd need to be lazy loaded to avoid perf issues. Unless they are already loaded.. @lucasbordeau what do you think, would that be complex?

harshrajeevsingh commented 3 weeks ago

@FelixMalfait Yes, there are more than 4k icons. The performance issue can be fixed using a virtualization library like react-window, but the User Experience would be awful. We should take a different approach.

lucasbordeau commented 3 weeks ago

1- We should look into what's distributed by Vite in real time by comparing what you did with the base 25-icon version. 2- We should test that in production build also 3- We already implement windowing in other parts of the app, I don't see a big problem doing it on this component

From a technical perspective I don't see anything preventing us from doing it.

@Bonapara could you give us a product insight about this ? Infinite scroll + search vs fixed number of icons + search ? Or something else ?

Bonapara commented 3 weeks ago

We can do infinite scroll with lazy loading + change the search to Search 4000+ icons and still limit the height to 25 icons max.

lucasbordeau commented 3 weeks ago

Ok let's implement windowing + lazy loading then + the UX changes that you gave.

@BOHEUS Could you update your issue to reflect that ?

BOHEUS commented 3 weeks ago

@lucasbordeau done