[Table and collection views] Consider content inset when calculating (default) element size range #525
Conversation
nguyenhuy
changed the title
…lating element size range
nguyenhuy
force-pushed
the
HNPagerContentInset
branch
from
64e0b09 to
f9d6b41
Compare
There was a problem hiding this comment.
This is a good change @nguyenhuy . One open question for you.
Source/ASPagerNode.m
Outdated
| CGSize pageSize = self.bounds.size; | ||
| UIEdgeInsets contentInset = self.view.contentInset; | ||
| if (! UIEdgeInsetsEqualToEdgeInsets(UIEdgeInsetsZero, contentInset)) { | ||
| pageSize.width -= (contentInset.left + contentInset.right); |
There was a problem hiding this comment.
Do we want to apply the horizontal inset here? It makes perfect sense to me that users would want to account for content inset in the cross direction, but I'm not so sure about the scrolling direction. For example, if they have a 50pt left inset so that the carousel starts off inset by a bit, they probably still want each page to be full-width, since the insets will only be applied at the start/end of the feed, not on every page. Thoughts?
There was a problem hiding this comment.
Oh, good point! I'll fix this.
| UIEdgeInsets contentInset = collectionView.contentInset; | ||
| if (! UIEdgeInsetsEqualToEdgeInsets(UIEdgeInsetsZero, contentInset)) { | ||
| maxSize.width -= (contentInset.left + contentInset.right); | ||
| maxSize.height -= (contentInset.top + contentInset.bottom); |
There was a problem hiding this comment.
Same principle as in the pager: I think we should not apply these in the scrolling direction.
| - (ASScrollDirection)flowLayoutScrollableDirections:(UICollectionViewFlowLayout *)flowLayout { | ||
| return (flowLayout.scrollDirection == UICollectionViewScrollDirectionHorizontal) ? ASScrollDirectionHorizontalDirections : ASScrollDirectionVerticalDirections; | ||
| } | ||
|
|
There was a problem hiding this comment.
This method isn't used anywhere.
| @@ -66,6 +66,7 @@ - (instancetype)init | |||
| - (instancetype)initWithCollectionViewLayout:(ASPagerFlowLayout *)flowLayout; | |||
| { | |||
| ASDisplayNodeAssert([flowLayout isKindOfClass:[ASPagerFlowLayout class]], @"ASPagerNode requires a flow layout."); | |||
| ASDisplayNodeAssertTrue(flowLayout.scrollDirection == UICollectionViewScrollDirectionHorizontal); | |||
There was a problem hiding this comment.
The existing implementation of this class (e.g -currentPageIndex and - scrollToPageAtIndex: animated:) assumes a horizontal scroll direction. Let's formalize that.
nguyenhuy
force-pushed
the
HNPagerContentInset
branch
from
a58820c to
1243609
Compare
| @@ -112,7 +113,15 @@ - (void)didLoad | |||
|
|
|||
| - (NSInteger)currentPageIndex | |||
| { | |||
| return (self.view.contentOffset.x / CGRectGetWidth(self.view.bounds)); | |||
| return (self.view.contentOffset.x / [self pageSize].width); | |||
There was a problem hiding this comment.
The old implementation works as expected, but let's not assume that self.view.bounds.width == self.pageSize.width.
|
🚫 CI failed with log |
…(default) element size range (TextureGroup#525) * Table and collection views to consider their content inset when calculating element size range * Update CHANGELOG * Address comments * -[ASPagerNode currentPageIndex] to use pageSize instead of bounds * Update documentation in ASPagerNode * Minor change
No description provided.