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(ItemWithIndex): remove ItemWithIndex type #129

Merged
merged 10 commits into from
Jun 10, 2024

Conversation

AlecColas
Copy link
Contributor

@AlecColas AlecColas commented May 29, 2024

An index was manually added to every iterable data passed to VirtualizedLists.
In convertToGrid function (dedicated to VirtualizedGrids), we saw that map gives access to the index of every item of data.

We can use the same for VirtualizedLists by using the index passed inside the renderItem prop.

The refactoring is cut in 2 parts :

  • Pass index to all components that use one (from bottom layers to top layers)
  • Delete ItemWithIndex.

Tests realized on Web :

web.mov

Some performance tests

Scenario Before After
Display Home avant affichage home affichage home
Scroll horizontally home avant horizontal home scroll horizontal home
Scroll vertically home avant vertical home scroll vertical home
Display Grid avant affichage grid affichage grid
Scroll Horizontal Grid avant horizontal grid scroll horizontal grid
Scroll Vertical Grid avant vertical grid scroll vertical grid

@AlecColas AlecColas added the enhancement New feature or request label May 29, 2024
@AlecColas AlecColas requested a review from pierpo May 29, 2024 10:40
@AlecColas AlecColas self-assigned this May 29, 2024
@pierpo
Copy link
Member

pierpo commented May 29, 2024

This is awesome, thank you so much ❤️ . I think there might be a bug though on the virtualized grid indices!

image (1)

vs

image

@AlecColas
Copy link
Contributor Author

AlecColas commented May 30, 2024

@pierpo I fixed it and updated the video in the PR's description

Copy link
Member

@pierpo pierpo left a comment

Choose a reason for hiding this comment

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

Awesome, thanks ❤️

@pierpo pierpo merged commit 04c1455 into main Jun 10, 2024
1 check passed
@pierpo pierpo deleted the refactor/remove-indexed-items branch June 10, 2024 08:00
pierpo pushed a commit that referenced this pull request Jun 10, 2024
* feat(virtualizedList): pass index in props of itemContainerWithAnimatedStyle

* feat(virtualizedListWithNodes): pass index in props of itemWrapperWithVirtualParentContext

* feat(virtualizedListWithScroll): pass index as props of itemWrapperWithScrollContext

* feat(virtualizedGrid): pass index in renderHeader, GridRow then ItemWrapper

* refactor(virtualizedGrid): stop using ItemWithIndex type

* refactor(virtualizedList): stop using ItemWithIndex type in layers of list

* refactor(virtualizedList): delete ItemWithIndex type

* refactor(SNvirtualizedList): stop indexing data in first layer

* refactor(Lists): delete addIndex helper

* fix(virtualizedGrid): fix index of row by taking header into account
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants