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

[Android] Deprecate RecyclerViewBackedScrollView #11095

Closed
wants to merge 1 commit into from

Conversation

mkonicek
Copy link
Contributor

@mkonicek mkonicek commented Nov 23, 2016

We don't use this component internally and seems like it's not needed.

From the docs of this component it looks like there's no point in its existence:

* It simply renders rows passed as children in a separate recycler view cells
* similarly to how `ScrollView` is doing it. Thanks to the fact that it uses
* native `RecyclerView` though, rows that are out of sight are going to be
* automatically detached (similarly on how this would work with
* `removeClippedSubviews = true` on a `ScrollView.js`).

I'd like to deprecate this component we don't use it at fb and it has a maintenance cost, e.g.: #7002

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @lebronJ and @tegon to be potential reviewers.

@mkonicek
Copy link
Contributor Author

mkonicek commented Nov 23, 2016

After some discussion a better option seems to move this to a separate repo as we're not using this internally at fb but some people use this.

@mkonicek mkonicek closed this Nov 23, 2016
@@ -58,6 +64,14 @@ var RecyclerViewBackedScrollView = React.createClass({

mixins: [ScrollResponder.Mixin],

componentWillMount: function() {

Choose a reason for hiding this comment

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

no-undef: 'log' is not defined.

facebook-github-bot pushed a commit that referenced this pull request Dec 16, 2016
Summary:
cc brentvatne

potential reviewers mkonicek and kmagiera

**Motivation for making this change.**

The previous PR was closed : #11095 but the followup actions was never done

I reopened a really similar one so it get merged
RecyclerView is no more used at Facebook (according to previous PR)

According to brentvatne, their were two motivations for RecyclerView:
* ListView with ScrollView component used to bounce back on row insert, but this is now fixed
* This made possible to implement certain performance improvements, but the maintenance cost was not worth the risk

With RN 0.37, the actual code in React Native make the app crash:
- see #10560

I spend one hour investigating this and did also require brent time at exponent slack. I think other people are struggling too.

**Test plan**

<img width="708" alt="screen shot 2016-12-13 at 23 42 22" src="https://cloud.githubusercontent.com/assets/13785185/21162483/dbeb642e-c18d-11e6-9c32-1fe73f1826c1.png">

**Code formatting**

The
Closes #11445

Differential Revision: D4340640

Pulled By: mkonicek

fbshipit-source-id: 64c5cf060f2eb035d4d6199b30f0e73afc520180
DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017
Summary:
cc brentvatne

potential reviewers mkonicek and kmagiera

**Motivation for making this change.**

The previous PR was closed : facebook#11095 but the followup actions was never done

I reopened a really similar one so it get merged
RecyclerView is no more used at Facebook (according to previous PR)

According to brentvatne, their were two motivations for RecyclerView:
* ListView with ScrollView component used to bounce back on row insert, but this is now fixed
* This made possible to implement certain performance improvements, but the maintenance cost was not worth the risk

With RN 0.37, the actual code in React Native make the app crash:
- see facebook#10560

I spend one hour investigating this and did also require brent time at exponent slack. I think other people are struggling too.

**Test plan**

<img width="708" alt="screen shot 2016-12-13 at 23 42 22" src="https://cloud.githubusercontent.com/assets/13785185/21162483/dbeb642e-c18d-11e6-9c32-1fe73f1826c1.png">

**Code formatting**

The
Closes facebook#11445

Differential Revision: D4340640

Pulled By: mkonicek

fbshipit-source-id: 64c5cf060f2eb035d4d6199b30f0e73afc520180
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

3 participants