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

[Remove IGListCollectionView] Should IGListAdapter really depend on IGListCollectionView? #409

Closed
ocrickard opened this issue Jan 12, 2017 · 13 comments
Assignees
Milestone

Comments

@ocrickard
Copy link

There are legitimate reasons to use a different UICollectionView, and virtually every large codebase that looks at incremental conversion will have to use IGListAdapter with an existing subclass of UICollectionView. I get the need to use this for people new to the framework, but we need a way to configure IGListKit to work with existing collection views. Would we reconsider the dependency?

@jessesquires
Copy link
Contributor

This has come up before: #240.

I'm open to removing IGLK_SUBCLASSING_RESTRICTED. Would that be sufficient?

@jessesquires jessesquires changed the title Should IGListAdapter really depend on IGListCollectionView Should IGListAdapter really depend on IGListCollectionView? Jan 12, 2017
@jessesquires
Copy link
Contributor

Actually, one other thought / data point here.

If users wanted to use IGListKit together with something like JSQMessages, this currently would not be possible since JSQ has it's own UICollectionView subclass. (Although, this will be removed from JSQ in the next major release.)

@ocrickard
Copy link
Author

ocrickard commented Jan 13, 2017

Yep, I saw that, but I'm not proposing allowing subclassing. I don't see why this can't be used with an arbitrary UICollectionView subclass, and I don't want to make subclassing the defined class in the framework the only way to use a custom collection view. There are numerous valid uses of subclasses.

Is there a good reason to keep the collection view in the interface of IGListAdapter? If people want to use IGListCollectionView, great.

@jessesquires
Copy link
Contributor

jessesquires commented Jan 13, 2017

@ocrickard 👍

Primary reason for the class is just to attempt to prevent access to UICollectionView APIs that are "invalid" under IGListKit.

The only real negative to removing this is that users will be able to misuse the framework more easily. But, that can be (partially) remedied with docs.

@rnystrom
Copy link
Contributor

Talked w/ @ocrickard a bit about this today. I'm warming up to the idea.

I really don't want to lose the compiler safety of not calling update methods, but at the same time the pain is real for some folks. Hell even facebookarchive/AsyncDisplayKit#2848 had to hack around it.

I've also been brainstorming adding UITableView support down the road, so this would be a good first step.

And I agree if we're going to do this let's just drop IGListCollectionView altogether and use UICollectionView

@jessesquires
Copy link
Contributor

One more thing, if you want to use UICollectionViewController (which is common), then IGListCollectionView prevents that. (unless you do that ASDK hack, which is gross)

@rnystrom
Copy link
Contributor

@jessesquires oh nice, that's a great point!

@jessesquires
Copy link
Contributor

@rnystrom - what's the decision here?

kill IGListCollectionView ?

@jessesquires
Copy link
Contributor

Or -- we could keep IGListCollectionView and recommend it's use.

However, other APIs could change to UICollectionView.

This would also make it easier to make this change in IG

@rnystrom
Copy link
Contributor

I think we should kill it. There are legit reasons to subclass UICollectionView (e.g. AsyncDisplayKit, iOS compatibility hacks, API extensions) and we're restricting others. The only downside is losing the compiler errors on mutating the collection view directly. Really wish there would be a way to enforce that w/out huge hacks...

@jessesquires jessesquires self-assigned this Mar 2, 2017
@jessesquires
Copy link
Contributor

imma do dis

@jessesquires
Copy link
Contributor

landing internally now 🛬

facebook-github-bot pushed a commit that referenced this issue Mar 6, 2017
Summary: Remove `IGListCollectionView` per #409. Use plain old `UICollectionView`.

Reviewed By: rnystrom

Differential Revision: D4640425

fbshipit-source-id: 871b75eaeb1c9f2a40fe8f3fd81b209661704587
@rnystrom rnystrom closed this as completed Mar 6, 2017
facebook-github-bot pushed a commit that referenced this issue Mar 7, 2017
Summary: This reverts commit 871b75eaeb1c9f2a40fe8f3fd81b209661704587

Differential Revision: D4640425

fbshipit-source-id: 4b0e8a9820891062cf7f8d13de13d678adb5df4a
@jessesquires jessesquires reopened this Mar 8, 2017
@jessesquires
Copy link
Contributor

Taking another stab at this today.

facebook-github-bot pushed a commit that referenced this issue Mar 31, 2017
Summary:
Take 2. Remove `IGListCollectionView` per #409. Use plain old `UICollectionView`.
(This re-applies D4640425 and updates as needed.)

Also:
- run `pod update` everywhere
- update changelog

Reviewed By: paulvanderspek

Differential Revision: D4812207

fbshipit-source-id: 1ddbae06cdeddb43d8af175d3e8a045a36ff150e
@jessesquires jessesquires changed the title Should IGListAdapter really depend on IGListCollectionView? [Remove IGListCollectionView] Should IGListAdapter really depend on IGListCollectionView? Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants