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

disables prefetchEnabled by default #323

Closed
wants to merge 1 commit into from
Closed

disables prefetchEnabled by default #323

wants to merge 1 commit into from

Conversation

svenbacia
Copy link
Contributor

Changes in this pull request

Disables prefetchEnabled by default on IGListCollectionView (#318).

Pull request checklist

  • All tests pass. Demo project builds and runs.
  • I added tests, an experiment, or detailed why my change isn't tested.
  • I have reviewed the contributing guide

Copy link
Contributor

@jessesquires jessesquires left a comment

Choose a reason for hiding this comment

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

Awesome 🎉

Thanks @svenbacia !

[self updateIGListCollectionViewDefaultValues];
}
return self;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

ha 😅 we removed initWithCoder: previously so that we don't override storyboard settings.

let's remove, but let's keep your tests (asserts will need to be flipped)

return self;
}

- (void)updateIGListCollectionViewDefaultValues {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename: updateWithDefaultValues

@jessesquires
Copy link
Contributor

Ha, sorry. Just saw @rnystrom 's comments in #318.

@jessesquires
Copy link
Contributor

Actually, it feels bad to override storyboard settings. We probably shouldn't...

Maybe we only do it for the prefetch property?

cc @rnystrom

@jessesquires
Copy link
Contributor

1 more thing - @svenbacia, can you also update the CHANGELOG ? 😄

@rnystrom
Copy link
Contributor

rnystrom commented Dec 12, 2016

@svenbacia +1 only changing the prefetchingEnabled value in init. Let's leave the rest of the settings alone.

I took a look and storyboards do have this as an option. However I think it's still worth disabling since you can't actually use the feature.

@jessesquires maybe we should override:

-(id<UICollectionViewDataSourcePrefetching>)prefetchDataSource {
  return nil;
}

Maybe assert that it isn't used? Or maybe we add compiler warnings like we do with other illegal methods?

screen shot 2016-12-12 at 6 47 32 pm

@jessesquires jessesquires added this to the 2.1.0 milestone Dec 13, 2016
@coveralls
Copy link

coveralls commented Dec 13, 2016

Coverage Status

Coverage increased (+0.01%) to 97.429% when pulling 39dbeea on svenbacia:feature/disable-prefetch-by-default into 478264c on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@svenbacia updated the pull request - view changes

@rnystrom
Copy link
Contributor

Awesome! @svenbacia just need to rebase then we can land.

@facebook-github-bot
Copy link
Contributor

@svenbacia updated the pull request - view changes

@svenbacia
Copy link
Contributor Author

Done :)

@facebook-github-bot
Copy link
Contributor

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

@coveralls
Copy link

coveralls commented Dec 13, 2016

Coverage Status

Coverage increased (+0.01%) to 97.429% when pulling 842fa84 on svenbacia:feature/disable-prefetch-by-default into 626d268 on Instagram:master.

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

Successfully merging this pull request may close these issues.

5 participants