-
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
Vertical UICollectionViewLayout supporting inline sections #450
Conversation
naming nit: how about just |
@jessesquires I dig it. I left "vertical" b/c atm it only supports v-scrolling, but we could maybe add horizontal down the road. Or not. I like that better. |
@rnystrom updated the pull request - view changes |
@rnystrom updated the pull request - view changes |
@jeremycohen how do you feel about me landing this and setting up our internal tests? This is copy-pasted and cleaned up from D4455662. |
|
||
Please see the unit tests for more configuration examples and expected output. | ||
*/ | ||
IGLK_SUBCLASSING_RESTRICTED |
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.
Can we allow subclass here? Subclassing UICollectionViewFlowLayout
proved to be useful.
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.
Hmm, maybe we could. It's not really designed to be subclassed, but I guess we could let people do that if they wanted. @jessesquires any thoughts?
Source/IGListCollectionViewLayout.h
Outdated
- (CGFloat)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)collectionViewLayout minimumInteritemSpacingForSectionAtIndex:(NSInteger)section; | ||
- (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)collectionViewLayout referenceSizeForHeaderInSection:(NSInteger)section; | ||
|
||
Sections and items are put into the same horizontal row until the max-y position of an item extends beyond the 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.
Sections and items are put into the same horizontal row until the
max-y
position of an item extends beyond the width
Should be max-x here, right?
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.
Ooops ya
Source/IGListCollectionViewLayout.mm
Outdated
|
||
const IGSectionEntry section = _sectionData[sectionCount - 1]; | ||
const CGFloat height = CGRectGetMaxY(section.bounds) + section.insets.bottom; | ||
return CGSizeMake(MIN(CGRectGetWidth(self.collectionView.bounds), self.maximumContentWidth), height); |
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.
Not sure if we should take collectionView.contentInset
into account here.
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.
It is my understanding that maximumContentWidth
acts sort of like collectionView.contentInset
here so that we can have paddings on left and right. So, we do not take contentInset
into account in this layout at all? However, we don't allow setting bottomInset in this layout. It is a little bit confusing.
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 think that collectionView.contentInset
is used by the actual scroll view and isn't used in the layout calculations. For instance in Instagram we use a variation of this layout and set the content inset to sit underneath the nav bar and it works. But that's a good point I haven't really considered that before. I'm not sure if the layout is supposed to consider that... 🤔
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.
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.
👍👍👍
@rnystrom updated the pull request - view changes - changes since last import |
@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@rnystrom updated the pull request - view changes - changes since last import |
@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
1 similar comment
@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Any chance for an example which this new layout will bring us? |
This is exactly what I need, arriving at just the right time! |
@weyert yup! We'll add one shortly. Still ironing out some internal kinks as we land this. |
Had a hiccup where we had to unland this, adding back ASAP |
Thank you @rnystrom Does it mean you will support horizontal scrolling in inline controllers? |
@weyert you can use embedded lists or scroll views to achieve an h-scroll, inline effect. If you want to use h-scrolling section controllers, check out our example. |
Summary: Follow from #450, this layout has been replaced. You served us well! - [x] All tests pass. Demo project builds and runs. - [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. Closes #482 Differential Revision: D4553897 Pulled By: rnystrom fbshipit-source-id: caccb386ba8914fbbf5e81d997524efb78c6e4ea
@rnystrom Aha, so you need to make a collection view cell which in itself contains a UICollectionView instance again? I have to admit that I am really stupid and lost the clue by now. What's the added advantage of inline sections? Somehow I thought it was to add things like horizontal scrolling items. |
@weyert oh sorry, I should have explained better. Inline sections are nice b/c Hope that explains it! |
Working on porting our collection view layout to IGListKit. I'm doing this because its a solid layout, and we just finished preparing it to work with inline sections. It is designed to work in tandem with IGListKit, so we're adding it.
This is still a WIP as I add more tests, but I'd love as much feedback as possible.
Aside from the glob of header documentation, this has the following features:
Followup to #423
TODO
Move decoration view support to delegateremovedUpdateinitWithCoder:
to use sensible defaultsinitWithCoder:
throwsMoveremoveddecorationBorderViewOfKind:
internal? C function? (or add unit tests)