[ASTableView] Add constrainedSizeForRowAtIndexPath: to control row heights from delegate

[hannahmbanana]

Addresses this suggestion: #1242

[levi]

Thoughts on adding this to the delegate instead?

[levi]

Relevant PR: #1740

[hannahmbanana]

@levi @maicki: I put it in the data source to be consistent with the current state of ASCollectionView. Should I switch it to the delegate assuming #1740 will eventually be committed?

[appleguy]

I had discussed this with Hannah. I suggested the Data Source because it is unclear to me if we will land the other PR. If we do, unfortunately we will need to introduce code that detects the method existing on the data source. Otherwise it is a very problematic “silent” error that could cause a lot of grief for app developers who update to 2.0, but don’t happen to test a deep corner of their app that uses this method. I’ve seen in the community it is quite common to not set the .delegate, whether or not this is advisable (it is supported by Apple).

There is also a close parallelism between the nodeFor: and constrainedSizeFor: methods, but ultimately we need to make a decision on the other PR’s course to landing safely before deciding if this should also be in the delegate.

On Jun 18, 2016, at 1:24 PM, Hannah Troisi notifications@github.com wrote:

@levi https://github.com/levi @maicki https://github.com/maicki: I put it in the data source to be consistent with the current state of ASCollectionView. Should I switch it to the delegate assuming #1740 #1740 will eventually be committed?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub #1769 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAigA5jRVZ85I6uuJP_YiWgHOKHynvEfks5qNFPrgaJpZM4I41cp.

[levi]

@appleguy the other diff was updated to introduce the data source detection note. If you're concerned about a breaking change, we can come up with a plan to assert or warn of the deprecation in an upcoming 2.0 beta and introduce a porting guide to 2.0 when we finalize the release.

The benifit I see here of having these methods on the delegate is that they're are optional and geometry-defining. A quick glance at UITableViewDataSource and UICollectionViewDataSource will show that there is a strict set of responsibilities for the data source that break down to defining count, data, and responding to insertion/deletion events. The delegate protocols, on the other hand have a slew of geometry-based methods, defining size, insets — much like our sizeForNode methods. I think there is some benifit here keeping the data source light and moving optional methods like this to the delegate. It will keep the data source simple and clear on what needs to be defined to support a functional list of nodes.

[maicki]

@appleguy I agree with @levi here. I would suggest moving this method to the delegate to be aligned with UITableViewDataSource as well as to have the responsibility on the right place: A DataSource and Delegate have specific responsibilities and the responsibilities for sizing lays in the delegate.

I also think we should move the same method slowly to ASCollectionViewDelegate as the other diff does with 2.0. Like @levi said via a slow deprecation and a porting guide.

I will be a bit confusing before 2.0 to have the same method on different protocols between table and collection view, but it is getting even more confusing by introducing this method in the data source and deprecate it slowly after.

[hannahmbanana]

Looks like the consensus is start the table method out in the delegate (non-consistent protocol placements between table & collection) and then later move the collection method to the delegate.

[maicki]

As you moved the methods to the delegate can you also change all of the names in the test from delegate to dataSource please.

[appleguy]

Thanks for adding tests!

[appleguy]

@levi @maicki you make a good case. I certainly agree it's more appropriate there, just wondering what the path is to move the other method as it has several considerations. Let's go with this route, and sign up for full compatibility with the collection view method existing on the .dataSource only. Thanks @hannahmbanana !