-
Notifications
You must be signed in to change notification settings - Fork 57
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
Fixed the invalidating heights animation #164
Conversation
Source/Cells/BrickCell.swift
Outdated
@@ -218,6 +218,12 @@ open class BrickCell: BaseBrickCell { | |||
_brick.brickCellTapDelegate?.didTapBrickCell(self) | |||
} | |||
|
|||
open override func apply(_ layoutAttributes: UICollectionViewLayoutAttributes) { | |||
UIView.animate(withDuration: UIView.inheritedAnimationDuration, animations: { |
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 shouldn't be a UIView.animate
for every apply
96a669d
to
4ca390e
Compare
@@ -26,6 +26,7 @@ | |||
buildConfiguration = "Debug" | |||
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB" | |||
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB" | |||
language = "" |
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.
Is this needed?
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 this is an artifact from Xcode 9
} | ||
|
||
let size = BrickSize(width: .ratio(ratio: 1), height: newHeight) | ||
self.brickCollectionView.resizeBrick(with: BrickIdentifiers.titleLabel, newSize: size) |
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 wrap it in an animation block? That way it's a good documentation on how to animate this
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 the idea is that its already implicitly animated since it delegates to performUpdates function which also does this. wrapping it in an animation block doesn't have any effect if you just want the default animation parameters
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.
Correct, there is a default animation, warping it in an animation block only changes the the duration.
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 understand, but it would be good to demonstrate how to change the duration (and animation options)
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.
Ok, no big deal
@@ -234,7 +234,27 @@ open class BrickCollectionView: UICollectionView { | |||
self.collectionViewLayout.invalidateLayout(with: BrickLayoutInvalidationContext(type: .invalidate)) | |||
}, completion: completion) | |||
} | |||
|
|||
|
|||
open func resizeBrick(with identifier: String, newSize: BrickSize, completion: ((Bool) -> Void)? = nil) { |
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 also supply an optional index as well?
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.
Sure
brick.size = newSize | ||
} | ||
|
||
self.collectionViewLayout.invalidateLayout(with: BrickLayoutInvalidationContext(type: .invalidate)) |
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.
Why is this needed?
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's not, removing
|
||
open func resizeBrick(with identifier: String, newSize: BrickSize, completion: ((Bool) -> Void)? = nil) { | ||
self.performBatchUpdates({ | ||
if let indexPath = self.indexPathsForBricksWithIdentifier(identifier).first { |
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.
We should hold a reference to the fetched indexPahts, so we can use them later. Also, why are we only doing first?
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.
The index path may have changed after the invalidateLayout so we have to get it again
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.
Why would the indexPaths have changed?
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 would because we we calling invalidate layout but now since we don't that that's no longer an issue
open override func preferredLayoutAttributesFitting(_ layoutAttributes: UICollectionViewLayoutAttributes) -> UICollectionViewLayoutAttributes { | ||
guard self._brick.height.isEstimate(withValue: nil) else { | ||
return layoutAttributes | ||
} | ||
|
||
let preferred = layoutAttributes.copy() as! UICollectionViewLayoutAttributes | ||
let preferred = layoutAttributes |
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.
Should we revert this?
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.
Doesn't seem to have any side effects by the looks of it
4ca390e
to
3359fd5
Compare
Codecov Report
@@ Coverage Diff @@
## master #164 +/- ##
=======================================
Coverage 93.42% 93.42%
=======================================
Files 39 39
Lines 4107 4107
Branches 333 332 -1
=======================================
Hits 3837 3837
Misses 269 269
Partials 1 1
Continue to review full report at Codecov.
|
3359fd5
to
4a3b004
Compare
|
||
open func resizeBrick(with identifier: String, index: Int? = nil, newSize: BrickSize, completion: ((Bool) -> Void)? = nil) { | ||
self.performBatchUpdates({ | ||
if let indexPath = self.indexPathsForBricksWithIdentifier(identifier, index: index).first, |
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.
Why are we only taking the first one? Should we just make index non-optional then?
Source/Bricks/Image/ImageBrick.swift
Outdated
@@ -149,7 +149,7 @@ open class ImageBrickCell: GenericBrickCell, Bricklike, AsynchronousResizableCel | |||
fileprivate var imageLoaded = false | |||
fileprivate var currentImageURL: URL? = nil | |||
|
|||
@IBOutlet public weak var imageView: UIImageView! | |||
@IBOutlet weak var imageView: UIImageView! |
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.
Why was this changed?
4a3b004
to
0ac6344
Compare
…-archive#164. This fixes issue where scrolling in a carousel that used collection brick caused cells above to re-size to a smaller size than what was already calculated
Closed #153