Skip to content
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

Refactor VirtualList to typescript #5307

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Refactor VirtualList to typescript #5307

wants to merge 2 commits into from

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Jul 3, 2024

This reverts commit 5d331b9.

(cherry picked from commit 4f7e7d9)

Recreated #5255 because that one became messed up after google drive merge to main

TODO

This reverts commit 5d331b9.

(cherry picked from commit 4f7e7d9)
@mifi mifi changed the title Revert "reverse virtuallist refactor" Refactor VirtualList to typescript Jul 3, 2024
Copy link
Contributor

github-actions bot commented Jul 3, 2024

Diff output files
No diff

@lakesare lakesare self-requested a review July 3, 2024 12:27
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not our code, we're never going to upstream this change, so is this a good idea? If we want types, we could vendor https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/preact-virtual-list/index.d.ts

@lakesare
Copy link
Contributor

lakesare commented Jul 5, 2024

@aduh95, the original library has last been updated 8 years ago, we're effectively on our own now.
Also - we tweaked it many times already, even the initial Renee's commit included tweaks.

I'm pro refactoring it into typescript & treating it like our own code.

@mifi
Copy link
Contributor Author

mifi commented Jul 7, 2024

i'm open to switching to some module to do list virtualization, like for example https://tanstack.com/virtual/latest - however if we'ere going to instead keep our own implementation I think we should indeed refactor it to typescript

Co-authored-by: Antoine du Hamel <[email protected]>
@lakesare
Copy link
Contributor

To be clear about my pending review - I'll review it when the performance part of this this PR ("VirtualList seems to be broken (fix it)") is implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants