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

Allow providing an external scrollview. #502

Merged
merged 6 commits into from
Jul 6, 2022
Merged

Conversation

gorhom
Copy link
Contributor

@gorhom gorhom commented Jul 3, 2022

Description

Fixes (issue #)

This PR will allow passing an external scrollview to the recyclerlistview, which is needed to allow gesture interactions with react-native-gesture-handler especially on Android.

Reviewers’ hat-rack 🎩

  • Prop description and docs.

Screenshots or videos (if needed)

Before

Screen.Recording.2022-07-03.at.15.48.05.mov

After

Screen.Recording.2022-07-03.at.15.47.18.mov

Checklist

@ghost ghost added the cla-needed label Jul 3, 2022
@nandorojo
Copy link

Thoughts on renaming the prop to ScrollViewComponent?

@naqvitalha
Copy link
Collaborator

@gorhom VirtualizedList already has a prop called renderScrollComponent. I'd suggest we reuse the same contract to make sure FlashList remains closer to default lists. Thoughts?

@ghost ghost removed the cla-needed label Jul 4, 2022
@gorhom
Copy link
Contributor Author

gorhom commented Jul 4, 2022

@naqvitalha sounds better! i have updated the pr 👍

Copy link
Collaborator

@naqvitalha naqvitalha left a comment

Choose a reason for hiding this comment

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

Few more comment. Looks good otherwise.

* Rendered as the main scrollview. Its contract for the scroll event should match the native scroll event contract, i.e.
* `scrollEvent = { nativeEvent: { contentOffset: { x: offset, y: offset } } }`
*/
renderScrollComponent?:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use default scroll view props from RN. That should be fine. All this complexity in RLV is due to its web only implementation. FlashList doesn't depend on it so using ScrollView props should be fine.

@@ -350,6 +351,7 @@ class FlashList<T> extends React.PureComponent<
windowCorrectionConfig={this.getUpdatedWindowCorrectionConfig()}
itemAnimator={this.itemAnimator}
suppressBoundedSizeException
externalScrollView={renderScrollComponent as any}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typecast to BaseScrollView instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i had to cast it to RecyclerListViewProps["externalScrollView"] instead

    externalScrollView?: {
        new (props: ScrollViewDefaultProps): BaseScrollView;
    };

@@ -96,6 +96,14 @@ estimatedItemSize?: number;

---

### `renderScrollComponent`
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Props are sorted alphabetically. Moving this would be helpful.

@@ -10,6 +10,7 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
## [1.0.4] - 2022-07-02

- Build fix for Android projects having `kotlinVersion` defined in `build.gradle`.
- Allow providing an external scrollview.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add link to this PR here

@naqvitalha
Copy link
Collaborator

@gorhom we plan to rollout a new version tomorrow. It would be great if you can address the comments and we pull this in :)

@gorhom
Copy link
Contributor Author

gorhom commented Jul 6, 2022

@naqvitalha i have addressed you comments in the latest commit 👍

Copy link
Collaborator

@naqvitalha naqvitalha left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks great!

@naqvitalha naqvitalha merged commit 5c6fb9a into Shopify:main Jul 6, 2022
@alantoa alantoa mentioned this pull request Aug 11, 2022
2 tasks
@RoccoDocco
Copy link

Using this prop this way:
renderScrollComponent={props => <ScrollView {...props} />}
I get this warning:
Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?

@sdfricke
Copy link

Using this prop this way: renderScrollComponent={props => <ScrollView {...props} />} I get this warning: Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?

I'm seeing the same issue

@hirbod
Copy link
Contributor

hirbod commented Nov 21, 2023

const ScrollViewWithRef = React.forwardRef((props, ref) => (
  <ScrollView {...props} ref={ref} />
)) as typeof ScrollView;

@sdfricke
Copy link

sdfricke commented Nov 22, 2023

const ScrollViewWithRef = React.forwardRef((props, ref) => (
  <ScrollView {...props} ref={ref} />
)) as typeof ScrollView;

Thank you for the suggestion. I'm not quite grasping how that would be implemented in the context of this simple example:
renderScrollComponent={props => <ScrollView {...props} />}

I've tried a couple different ways but getting quite a few errors so I'm guessing I'm not implementing that suggestion as intended.

@hirbod
Copy link
Contributor

hirbod commented Nov 22, 2023

  1. import scrollview from react-native-gesture-handler
  2. use my snippet
  3. pass renderScrollComponent={ScrollViewWithRef}

Enjoy

@sdfricke
Copy link

  1. import scrollview from react-native-gesture-handler
  2. use my snippet
  3. pass renderScrollComponent={ScrollViewWithRef}

Enjoy

Thanks again for following up. I do have that same strategy implemented but still getting a warning:

Warning: Failed prop type: Invalid prop `externalScrollView` of type `object` supplied to `ProgressiveListView`, expected `function`.

@hirbod
Copy link
Contributor

hirbod commented Nov 22, 2023

Please create a repo on Snack so I can have a look

@sdfricke
Copy link

sdfricke commented Nov 28, 2023

@hirbod it turns out I was on an older version. It looks like the issue I was seeing was fixed in version 1.4.0 as described here: #616

Thanks again for your assistance!

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.

None yet

6 participants