[Android] Implement fading edges for ScrollView and it's dependent FlatList by andre-krueger · Pull Request #26163 · facebook/react-native

andre-krueger · 2019-08-23T10:45:11Z

Summary

This should add props for enabling horizontal and vertical fading
edges for Scrollview and FlatList.
These fading edges are used to communicate to the user that there is more content to see.

Changelog

[Android] [Added] - fading edges props to the FlatList and ScrollView components

Test Plan

Open the React Native test app and navigate to the FlatList section.
Enable the useFadingEdges switch and insert a number into Fading edge length.

facebook-github-bot · 2019-08-23T10:45:20Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up 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 the corporate CLA signed.

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

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

facebook-github-bot · 2019-08-23T10:59:39Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

sahrens · 2019-08-23T18:47:43Z

Oh man that options list is getting pretty large...can you make it slightly larger by adding "android-only" to this new option so people don't get confused on iOS and this it's a bug when nothing happens? Also, could you or someone you work with implement support for iOS as well? It would be awesome to have platform parity on as many features as possible. This is a bit old, but is probably still relevant: https://www.cocoanetics.com/2011/08/adding-fading-gradients-to-uitableview/

sahrens

nevermind, your OS === android check is probably fine.

sahrens · 2019-08-23T18:50:31Z

Oh I see you added an android check - that's fine.

facebook-github-bot

@sahrens is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2019-08-23T20:05:05Z

This pull request was successfully merged by André Krüger in f8a64f9.

^{When will my fix make it into a release? | Upcoming Releases}

elicwhite · 2019-08-23T20:36:14Z

Is there a need to have a distinction between horizontalFadingEdgesEnabled and verticalFadingEdgesEnabled? Could we just have fadingEdgesEnabled that fades the edges depending on which direction the scrollview is? And if we do that we only need fadingEdgeLength, since that defaults to 0 which is off that handles the option being enabled or disabled too.

That would make this go from needing three props, to only one. :)

sahrens · 2019-08-23T22:14:57Z

I agree, let's only have a single edge length prop. I'm reverting now.

…ollView and it's dependent FlatList" Summary: After some thought, we decided we don't need the flexibility of separate horizontal and vertical props - it would be much nicer to just have a single prop for the edge length and then the native code can enable the booleans as appropriate. Original PR: #26163 Original commit changeset: f72a9a890d90 Reviewed By: TheSavior Differential Revision: D16997468 fbshipit-source-id: 7973262287a7ec2cee5957f8dc1806a0f28c1432

andre-krueger · 2019-08-24T13:46:20Z

I did combine the props to a single one, but unfortunately, I don't feel too confident to also implement the feature for iOS (I might be able to do that in a follow-up, though).

elicwhite · 2019-08-24T16:29:29Z

Do we need this Boolean? Or can we just check whether the length is 0 for disabled, non zero for enabled?

Yeah, you're right. Fixed.

elicwhite

Thanks for the update, this is really close! Two more small nits.

elicwhite · 2019-08-25T15:24:35Z

This isn’t needed here anymore, right?

Yes, fixed.

elicwhite · 2019-08-25T15:26:22Z

Can you add a bit more description here? Assume that what you put here will be the sentence describing what this prop does in our websites api documentation.

Thanks for the feedback. I added more description.

elicwhite

Looks good! Approved for someone to land on Monday

elicwhite · 2019-08-27T04:52:56Z

Ah, just checking this one last time made me realize that if the prop is set to 0 it should become disabled again.

I think this needs the following, right?

else { view.setHorizontalFadingEdgeEnabled(false); }

In both files

Yeah, you're right, this might apply to dev environments where the prop is changed quite often. Maybe the fading edge length should also be reset?

elicwhite · 2019-08-27T05:01:01Z

Found one more thing that stood out to me. Sorry for the churn and the back and forth! We'll get this in!

sahrens · 2019-08-27T15:20:39Z

Did you verify that toggling the fade switch in the example correctly shows and hides the fade?

This should add props for enabling horizontal and vertical fading edges for Scrollview and FlatList. These fading edges are used to communicate to the user that there is more content to see.

andre-krueger · 2019-08-27T16:00:36Z

Yes, it works correctly. I'm now also resetting the fading edge length to zero if the fading edges are disabled, just in case.

sahrens · 2019-08-27T16:05:01Z

Ok great.

elicwhite · 2019-08-27T20:50:41Z

We are having some trouble landing this PR due to some internal tooling issues around having previously reverting this. I reached out to our Open Source team for support.

facebook-github-bot

@TheSavior is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot added Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Aug 23, 2019

react-native-bot added Component: FlatList Component: ScrollView labels Aug 23, 2019

andre-krueger force-pushed the implement-scrollview-fading-edges branch from 702084e to 1979c1f Compare August 23, 2019 10:57

analysis-bot suggested changes Aug 23, 2019

View reviewed changes

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 Aug 23, 2019

andre-krueger force-pushed the implement-scrollview-fading-edges branch from 1979c1f to f1213a1 Compare August 23, 2019 11:00

andre-krueger changed the title ~~[Android] Implement fading edges for Scrollview and it's dependent FlatList~~ [Android] Implement fading edges for ScrollView and it's dependent FlatList Aug 23, 2019

andre-krueger force-pushed the implement-scrollview-fading-edges branch from f1213a1 to 3bae7cc Compare August 23, 2019 11:03

sahrens approved these changes Aug 23, 2019

View reviewed changes

facebook-github-bot approved these changes Aug 23, 2019

View reviewed changes

facebook-github-bot closed this in f8a64f9 Aug 23, 2019

react-native-bot added the Merged This PR has been merged. label Aug 23, 2019

elicwhite reopened this Aug 24, 2019

andre-krueger force-pushed the implement-scrollview-fading-edges branch from 3bae7cc to 51acd31 Compare August 24, 2019 13:44

elicwhite reviewed Aug 24, 2019

View reviewed changes

andre-krueger force-pushed the implement-scrollview-fading-edges branch from 51acd31 to 841b6df Compare August 24, 2019 16:47

elicwhite reviewed Aug 25, 2019

View reviewed changes

andre-krueger force-pushed the implement-scrollview-fading-edges branch from 841b6df to de33eb8 Compare August 25, 2019 16:38

elicwhite approved these changes Aug 25, 2019

View reviewed changes

elicwhite reviewed Aug 27, 2019

View reviewed changes

andre-krueger force-pushed the implement-scrollview-fading-edges branch from de33eb8 to df6bb8a Compare August 27, 2019 07:11

Implement fading edges for Scrollview and it's dependent FlatList

b86efed

This should add props for enabling horizontal and vertical fading edges for Scrollview and FlatList. These fading edges are used to communicate to the user that there is more content to see.

andre-krueger force-pushed the implement-scrollview-fading-edges branch from df6bb8a to b86efed Compare August 27, 2019 15:59

sahrens approved these changes Aug 27, 2019

View reviewed changes

facebook-github-bot approved these changes Aug 27, 2019

View reviewed changes

facebook-github-bot closed this in 51aacd5 Aug 28, 2019

andre-krueger mentioned this pull request Mar 11, 2020

Preparation for React-Native 0.62: Add type for fadingEdgeLength prop DefinitelyTyped/DefinitelyTyped#43065

Closed

9 tasks

This was referenced Jun 17, 2020

Update the FlatList Component in Lists MLH-Fellowship/react-native#41

Open

fadingEdgeLength prop in FlatList doesn't work #29182

Closed

Conversation

andre-krueger commented Aug 23, 2019

Summary

Changelog

Test Plan

Uh oh!

facebook-github-bot commented Aug 23, 2019

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Aug 23, 2019

Uh oh!

sahrens commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sahrens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

react-native-bot commented Aug 23, 2019

Uh oh!

elicwhite commented Aug 23, 2019

Uh oh!

sahrens commented Aug 23, 2019

Uh oh!

andre-krueger commented Aug 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elicwhite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elicwhite left a comment

Choose a reason for hiding this comment

Uh oh!

elicwhite Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elicwhite commented Aug 27, 2019

Uh oh!

sahrens commented Aug 27, 2019

Uh oh!

andre-krueger commented Aug 27, 2019

Uh oh!

sahrens commented Aug 27, 2019

Uh oh!

elicwhite commented Aug 27, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sahrens commented Aug 23, 2019 •

edited

Loading

elicwhite Aug 27, 2019 •

edited

Loading