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

[ListViewDataSource] Simplify API: constructor accepts rows and provides default rowHasChanged #1763

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@brentvatne
Collaborator

brentvatne commented Jun 26, 2015

As per #1595

getInitialState() {
  var ds = new ListView.DataSource({rowHasChanged: (r1, r2) => true});
  return {
    dataSource: ds.cloneWithRows(['row 1', 'row 2']),
  };
},

render() {
  return (
    <ListView
      dataSource={this.state.dataSource}
      renderRow={(rowData) => <Text>{rowData}</Text>}
    />
  );
},

becomes

getInitialState() {
  return {
    dataSource: new ListView.DataSource({rows: ['row 1', 'row 2']});
  };
},

render() {
  return (
    <ListView
      dataSource={this.state.dataSource}
      renderRow={(rowData) => <Text>{rowData}</Text>}
    />
  );
},
this._cachedRowCount = countRows(this.rowIdentities);
// Dirty all rows and sections
this._calculateDirtyArrays([], [], []);

This comment has been minimized.

@brentvatne

brentvatne Jun 26, 2015

Collaborator

TODO: have the constructor accept prevData param, so we don't do extra work when cloning

@brentvatne

brentvatne Jun 26, 2015

Collaborator

TODO: have the constructor accept prevData param, so we don't do extra work when cloning

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 29, 2015

Contributor

This does make the API simpler. Only concern is that new folks might not realise that they can optimise by using rowHasChanged. This is different from before where it was explicitly declared in the beginning and hence known beforehand.

While we are at it, we can change rowHasChanged to rowShouldUpdate to be like React's componentShouldUpdate method.

Contributor

paramaggarwal commented Jun 29, 2015

This does make the API simpler. Only concern is that new folks might not realise that they can optimise by using rowHasChanged. This is different from before where it was explicitly declared in the beginning and hence known beforehand.

While we are at it, we can change rowHasChanged to rowShouldUpdate to be like React's componentShouldUpdate method.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jun 29, 2015

Collaborator

While we are at it, we can change rowHasChanged to rowShouldUpdate to be like React's componentShouldUpdate method.

I like this. Probably makes more sense as a separate PR. We'll want to keep the old method - and have it print a helpful warning - around for a release or two so that people have time to migrate.

Collaborator

ide commented Jun 29, 2015

While we are at it, we can change rowHasChanged to rowShouldUpdate to be like React's componentShouldUpdate method.

I like this. Probably makes more sense as a separate PR. We'll want to keep the old method - and have it print a helpful warning - around for a release or two so that people have time to migrate.

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jul 15, 2015

Contributor

Yeah, this interface definitely needs to be better. I'm wondering if maybe we should just make a completely new class, maybe ListView.SimpleDataSource, instead of cluttering up the already cluttered ListViewDataSource?

Some other changes I'd like to make are to make the data blob headers structured data, so you could do something like this:

  dataSourceWithSections: new ListView.SimpleDataSource({sections: [
    {header: 'h1', rows: ['row 1a', 'row 1b']},
    {header: 'h2', rows: ['row 2a']}
  ]}),
  dataSourceWithoutSections: new ListView.SimpleDataSource({rows: [
    'row 1',
    'row 2',
  ]}),

what do you think?

re: rowShouldUpdate, I think we should actually probably ditch that completely along with the StaticRenderers and rely entirely on componentShouldUpdate of the actual row components, rather than adding additional layers of magic. We could also potentially add some timing code in render and it takes a long time and the components don't have shouldComponentUpdate implemented, we could warn.

Contributor

sahrens commented Jul 15, 2015

Yeah, this interface definitely needs to be better. I'm wondering if maybe we should just make a completely new class, maybe ListView.SimpleDataSource, instead of cluttering up the already cluttered ListViewDataSource?

Some other changes I'd like to make are to make the data blob headers structured data, so you could do something like this:

  dataSourceWithSections: new ListView.SimpleDataSource({sections: [
    {header: 'h1', rows: ['row 1a', 'row 1b']},
    {header: 'h2', rows: ['row 2a']}
  ]}),
  dataSourceWithoutSections: new ListView.SimpleDataSource({rows: [
    'row 1',
    'row 2',
  ]}),

what do you think?

re: rowShouldUpdate, I think we should actually probably ditch that completely along with the StaticRenderers and rely entirely on componentShouldUpdate of the actual row components, rather than adding additional layers of magic. We could also potentially add some timing code in render and it takes a long time and the components don't have shouldComponentUpdate implemented, we could warn.

@sahrens sahrens self-assigned this Jul 15, 2015

@aleclarson

This comment has been minimized.

Show comment
Hide comment
@aleclarson

aleclarson Aug 13, 2015

Contributor

👍

Contributor

aleclarson commented Aug 13, 2015

👍

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Sep 20, 2015

Contributor

@brentvatne Missed this PR today. ListView API currently needs some setup to get started - this PR simplifies that. Great comments from @sahrens on how to take this forward.

Contributor

paramaggarwal commented Sep 20, 2015

@brentvatne Missed this PR today. ListView API currently needs some setup to get started - this PR simplifies that. Great comments from @sahrens on how to take this forward.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Sep 20, 2015

Collaborator

Good point @paramaggarwal - I totally forgot about this :p

Collaborator

brentvatne commented Sep 20, 2015

Good point @paramaggarwal - I totally forgot about this :p

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 16, 2016

Contributor

Hi there! This issue is being closed because it has been inactive for a while.

But don't worry, it will live on with ProductPains! Check out its new home: https://productpains.com/post/react-native/listviewdatasource-simplify-api-constructor-accepts-rows-and-provides-default-rowhaschanged

ProductPains helps the community prioritize the most important issues thanks to its voting feature.
It is easy to use - just login with GitHub.

Also, if this issue is a bug, please consider sending a PR with a fix. We rely on the community to provide
bugfixes as the core team is focused on performance.

Contributor

mkonicek commented Mar 16, 2016

Hi there! This issue is being closed because it has been inactive for a while.

But don't worry, it will live on with ProductPains! Check out its new home: https://productpains.com/post/react-native/listviewdatasource-simplify-api-constructor-accepts-rows-and-provides-default-rowhaschanged

ProductPains helps the community prioritize the most important issues thanks to its voting feature.
It is easy to use - just login with GitHub.

Also, if this issue is a bug, please consider sending a PR with a fix. We rely on the community to provide
bugfixes as the core team is focused on performance.

@mkonicek mkonicek added the Icebox label Mar 16, 2016

@mkonicek mkonicek closed this Mar 16, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 16, 2016

@brentvatne updated the pull request.

facebook-github-bot commented Mar 16, 2016

@brentvatne updated the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment