-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add horizontal scrolling support to IGListCollectionViewLayout #857
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind also adding a test demonstrating:
- Items moving to the next column while still in the same section
- Sections that don't break to new columns (so they stack vertically)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this exactly what UIEdgeInsetsInsetRect
does already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on using a switch(...)
instead so the compiler will error if something ever changes? It likely wont, just good for longterm maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 will do
Source/IGListCollectionViewLayout.mm
Outdated
|
||
static CGFloat CGSizeGetLengthInDirection(CGSize size, UICollectionViewScrollDirection direction) { | ||
return direction == UICollectionViewScrollDirectionVertical ? size.height : size.width; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 love these methods 👌
@"%@ 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love it!
|
||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.md
for any breaking changes, enhancements, or bug fixes.This PR generalizes the layout logic in
IGListCollectionViewLayout.mm
to handle horizontally scrolling layouts, mainly by generalizing references towidth
,height
,x
andy
to take scrolling direction into account. This changes the signature ofIGListCollectionViewLayout.init
as 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.