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

Fix/animated lists types #36292

Closed
wants to merge 5 commits into from

Conversation

jeongshin
Copy link
Contributor

@jeongshin jeongshin commented Feb 25, 2023

Summary

I was working on Animated.FlatList and found some types missing as below

Screen Shot 2023-02-25 at 4 32 43 PM

also Animated.SectionList

Screen Shot 2023-02-25 at 4 31 34 PM

So I refactored type definition for Animated.FlatList and Animated.SectionList using abstract class.

Changelog

[GENERAL] [FIXED] - add missing FlatList types for Animated FlatList
[GENERAL] [FIXED] - add missing SectionList types for Animated SectionList

Test Plan

Ran yarn test-typescript and yarn test-typescript-offline with no errors.

@facebook-github-bot
Copy link
Contributor

Hi @jeongshin!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 25, 2023
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@analysis-bot
Copy link

analysis-bot commented Feb 25, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,471,413 -1,060
android hermes armeabi-v7a 7,793,521 -1,266
android hermes x86 8,946,844 -1,483
android hermes x86_64 8,804,129 -1,450
android jsc arm64-v8a 9,105,268 -1,241
android jsc armeabi-v7a 8,302,211 -1,456
android jsc x86 9,155,600 -1,671
android jsc x86_64 9,414,658 -1,639

Base commit: 81dd3af
Branch: main

Copy link
Contributor

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

Can we factor this to reuse the imperative methods already defined for VirtualizedList, instead of duplicating this?

@jeongshin
Copy link
Contributor Author

jeongshin commented Feb 27, 2023

Can we factor this to reuse the imperative methods already defined for VirtualizedList, instead of duplicating this?

I tried to use abstract class to reuse the imperative methods.
Thank you 🙏

SectionListComponent,
SectionListProps,
} from '../Lists/SectionList';
import {NodeHandle} from '../ReactNative/RendererProxy';

Choose a reason for hiding this comment

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

NodeHandle is not in use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unused type. thank you.

@jeongshin jeongshin requested review from redpanda-bit and removed request for NickGerleman February 27, 2023 22:25
@redpanda-bit
Copy link

Do you think it is a good idea to make changes only on Animated.d.ts and not change Flatlist.d.ts and SectionList.d.ts? Otherwise this looks good to me.

@jeongshin
Copy link
Contributor Author

Do you think it is a good idea to make changes only on Animated.d.ts and not change Flatlist.d.ts and SectionList.d.ts? Otherwise this looks good to me.

I made changes only on Animated.d.ts to fix Animated.FlatList type definition not supporting Animated.FlatList method types which shares with FlatList. (scrollToIndex, scrollToItem, ... etc).

So, I fixed Animated.FlatList to extends FlatList types including method types.

Screen Shot 2023-03-01 at 6 22 15 AM

@facebook-github-bot
Copy link
Contributor

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Feb 28, 2023
@facebook-github-bot
Copy link
Contributor

@NickGerleman merged this pull request in ed39d63.

OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
I was working on `Animated.FlatList` and found some types missing as below

![Screen Shot 2023-02-25 at 4 32 43 PM](https://user-images.githubusercontent.com/64301935/221345457-74252131-5207-4e17-ad96-92221a915305.png)

also `Animated.SectionList`

![Screen Shot 2023-02-25 at 4 31 34 PM](https://user-images.githubusercontent.com/64301935/221345679-07ba862b-708e-400e-ac14-7d2156fcc1e8.png)

So I refactored type definition for `Animated.FlatList` and `Animated.SectionList` using `abstract class`.

## Changelog

[GENERAL] [FIXED] - add missing FlatList types for Animated FlatList
[GENERAL] [FIXED] - add missing SectionList types for Animated SectionList

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

Pull Request resolved: facebook#36292

Test Plan: Ran `yarn test-typescript` and `yarn test-typescript-offline` with no errors.

Reviewed By: lunaleaps

Differential Revision: D43673884

Pulled By: NickGerleman

fbshipit-source-id: 7ccab5997fa2f22226fb0e106672cee98e568ba4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants