Add horizontal scrolling support to IGListCollectionViewLayout #857
Conversation
|
@edmonston updated the pull request - view changes |
|
@edmonston in order to land this earlier than the 4.0 release, what about keeping the existing init but just call the new initializer with Just want to get this in for 3.1 or 3.2 and not have to hold it. We're trying to avoid breaking changes in minor releases. Thoughts? edit: This is AMAZING btw! |
Source/IGListCollectionViewLayout.mm
Outdated
| rect.origin.y + insets.top, | ||
| rect.size.width - insets.left - insets.right, | ||
| rect.size.height - insets.top - insets.bottom); | ||
| } |
There was a problem hiding this comment.
Isn't this exactly what UIEdgeInsetsInsetRect does already?
There was a problem hiding this comment.
Doh, I did not even know this function already existed...
Source/IGListCollectionViewLayout.mm
Outdated
| } | ||
|
|
||
| static CGFloat CGPointGetCoordinateInDirection(CGPoint point, UICollectionViewScrollDirection direction) { | ||
| return direction == UICollectionViewScrollDirectionVertical ? point.y : point.x; |
There was a problem hiding this comment.
Thoughts on using a switch(...) instead so the compiler will error if something ever changes? It likely wont, just good for longterm maintenance.
Source/IGListCollectionViewLayout.mm
Outdated
|
|
||
| static CGFloat CGSizeGetLengthInDirection(CGSize size, UICollectionViewScrollDirection direction) { | ||
| return direction == UICollectionViewScrollDirectionVertical ? size.height : size.width; | ||
| } |
| @"%@ of item %zi in section %zi must be less than container %.0f accounting for section insets %@", | ||
| self.scrollDirection == UICollectionViewScrollDirectionVertical ? @"Width" : @"Height", | ||
| item, section, CGRectGetLengthInDirection(contentInsetAdjustedCollectionViewBounds, fixedDirection), | ||
| NSStringFromUIEdgeInsets(insets)); |
|
|
||
| // find the lowest point in the section and add the bottom inset to find the next row's Y | ||
| nextRowY = MAX(nextRowY, CGRectGetMaxY(rollingSectionBounds) + insets.bottom); | ||
| // find the farthest point in the section and add the trailing inset to find the next row's coord |
There was a problem hiding this comment.
Even these comment updates are great!
| IGAssertEqualFrame([self cellForSection:0 item:0].frame, 20, 10, 45, 10); | ||
| IGAssertEqualFrame([self cellForSection:0 item:1].frame, 20, 20, 45, 20); | ||
| IGAssertEqualFrame([self headerForSection:1].frame, 80, 10, 10, 85); | ||
| IGAssertEqualFrame([self cellForSection:1 item:0].frame, 90, 10, 45, 30); |
There was a problem hiding this comment.
Great test, can see the items stacking vertically
|
Definitely @rnystrom! Makes total sense to preserve the old initializer and just add a new one for now, so it isn't breaking and doesn't have to wait to 4.0. When I update the I'll make this change, add the unit tests you suggested and make the other tweaks from the comments. Stay tuned. |
|
@edmonston we can make this a 3.1 enhancement 😁 |
|
@edmonston updated the pull request - view changes |
|
@edmonston updated the pull request - view changes |
|
@edmonston updated the pull request - view changes |
|
So, I did those changes to make this PR non-breaking (in the semver sense), and everything seemed fine -- except I just realized that I had altered one other element of the public-facing Should I just revert to the original name (referencing the 'Y') for compatibility's sake, even though it's not quite accurate for a horizontal layout, and then we fix it later in 4.0? Even nit-pickier: As it stands now, the |
|
@edmonston Maybe consider adding both properties and just return the value of the new property for |
|
@edmonston updated the pull request - view changes |
|
Hey @weyert that's a good suggestion. I've updated the PR so (I also changed two occurrences of @rnystrom lemme know if those new unit tests (I actually added 4) seem ok. |
|
@edmonston awesome! Don't worry about changing the DI, that shouldn't break anything. Even though the naming is inconsistent, let's keep the I'm out of the country this week and will land this when I'm back, just in case this breaks something I don't want to make people scramble while I'm gone! |
|
@edmonston updated the pull request - view changes |
|
@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Changes in this pull request
Issue fixed: #752
Checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.This PR generalizes the layout logic in
IGListCollectionViewLayout.mmto handle horizontally scrolling layouts, mainly by generalizing references towidth,height,xandyto take scrolling direction into account. This changes the signature ofIGListCollectionViewLayout.initas well as the names of a few properties, so it would be a breaking change.I added a couple of unit tests specifically for horizontal layouts -- but held off from adding a horizontal version of every unit test for this class, as it would basically double the number of tests. But if you want that, just let me know and I'm happy to do it.
Also let me know if you want me to add a demo VC to the Examples project that uses this new horizontal flow layout -- I have some demo code handy (I used it for testing), but didn't want to clutter up the PR if you didn't want/need it.