@orta => use the flow layout api for getting header and footer dimensions

[1aurabrown]

I strongly believe that we should subclass the FlowLayout delegate as it already has an api for headers and footers. This makes it way easier for us or anyone else to swap layouts on the same collection view without having to use different APIs to provide headers and footers for each layout. Everything I've read about creating your own layout says that you should subclass the Flow Layout.

Still more changes to come.

[ashfurrow]

Hmm. I definitely see where you're coming from – it was my first reaction when I saw the layout subclass. However, when I looked at the requirements of the library, I realized that it would be very difficult to make the flow layout do what we need it to.

The issue is in the way that flow layouts work. Flow layouts should be used in a line-based, breaking layout.

We'd need to somehow trick the layout that we want to break horizontally even though that's the same direction we scroll in. We'd also need to know exactly how many items are in the collection view ahead of time, and eigen uses dynamic loading of content, further complicating the math.

While it's always a good idea to consider using flow layouts instead of using layouts directly, I think that in this case, using a flow layout would be more work than doing all of the math ourselves.

[1aurabrown]

i've tested this out with complete changes and it works totally fine. we already are providing specific locations for each item.

[ashfurrow]

You've tested it out with the complete changes? Modifying the superclass of the layout? How much of the flow layout superclass' code are we using?

[ashfurrow]

Like, I see how the current changes in the PR would work, but I don't know that changing the layout's superclass just to be able to use the UICollectionViewDelegateFlowLayout is necessary.

[1aurabrown]

@AshFurrow sorry, yes i've tried this out with some other changes. we also have to provide a method for + (instancetype)layoutAttributesForSupplementaryViewOfKind:(NSString *)elementKind withIndexPath:(NSIndexPath *)indexPath which is required when subclassing layouts, though we never provided it. we can hold off on merging, travis is having issues.

with using the flow layout delegate, we get a bunch of header and footer functionality out of the box. it's not only that we would be using the standard api provided, there are also some internals that happen with a flow layout to determine to create the header and footer, as i understand it.

[orta]

sorry to be the @dblock but can I request some snapshots here :camera:

[1aurabrown]

yes, my git repos are in a weird state though so i'm working to get eigen and arcollectionviewmasonrylayout working with intermediate changes

On Tue, Jun 24, 2014 at 5:07 PM, Orta notifications@github.com wrote:

sorry to be the @dblock https://github.com/dblock but can I request some snapshots here [image: :camera:]

— Reply to this email directly or view it on GitHub https://github.com/AshFurrow/ARCollectionViewMasonryLayout/pull/16#issuecomment-47030967 .

Ꮮaura Ᏼrown

[ashfurrow]

@1aurabrown Yeah, I can see that. Using the header/footer stuff would be sweet. I'll wait until you have the code pushed.

@orta There should be existing snapshot test cases for this repo.

[1aurabrown]

everything looks good, I can write snapshot tests. here are screenshots from eigen of artwork favorites, the shows feed and the artist view's artwork section.

[ashfurrow]

Is this the finished PR? I thought from the branch name that we were changing the superclass of the masonry layout. I don't know that I'm comfortable with our delegate protocol extending the flow layout delegate protocol, but not have us extend the flow layout itself.

[dblock]

I haven't read the detail, however I see that this needs at least a CHANGELOG entry :)

So this has no behavior change and all snapshot tests should be passing (Travis doesn't agree, but that might be unrelated)? Or did I miss something?

[ashfurrow]

Yeah travis seems borked due to a failing pod install, but they were having issues yesterday.

[1aurabrown]

looking into it!

[dblock]

Do a pod update locally - I bet if you pod install you will get the same error.

[1aurabrown]

thanks @dblock that fixed the pod issue

[1aurabrown]

ok @orta @AshFurrow this is green now. the superclass and the corresponding delegate have been changed, which means we can use the built-in header and footer api and whatever internals exist in the superclass

[ashfurrow]

OK, I'm still not completely comfortable with the idea of subclassing UICollectionViewFlowLayout but ignoring the superclass' implementations of the two main methods in the layout, but I see how it makes things easier for section headers and footers.

[ashfurrow]

@1aurabrown can you update the podspec?

[1aurabrown]

I have some followup changes that go with this so i can update the podspec and changelog too and tag a new version when we feel everything is in order

Laura Brown

On Thursday, June 26, 2014 at 08:31, Ash Furrow wrote:

@1aurabrown (https://github.com/1aurabrown) can you update the podspec?

— Reply to this email directly or view it on GitHub (https://github.com/AshFurrow/ARCollectionViewMasonryLayout/pull/16#issuecomment-47219491).