-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[NT-669, NT-496] Shimmer Loading Extension #980
Conversation
@@ -117,6 +121,13 @@ final class PledgeShippingLocationViewController: UIViewController { | |||
) | |||
} | |||
|
|||
self.viewModel.outputs.isLoading |
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.
This output was still dangling but I'll add separate outputs for hiding and showing these.
} | ||
|
||
extension UIView { | ||
var shimmersWhenLoading: Bool { |
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 like to think of a way to not do this to all UIView
s.
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.
Actually a simpler way of doing this is probably just to have the conforming view return an array of views to add the shimmer to. Iβll make that change.
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.
Agree this would be nicer π
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.
Also the fewer associated objects the better π
override func layoutSubviews() { | ||
super.layoutSubviews() | ||
|
||
self.layoutGradientLayers() |
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 always going to be necessary to call this on the ShimmerLoading
-conforming view, 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.
Hmm, yeah because we can't override layoutSubviews
in an extension π€
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.
Looks good! I think it would be nice not to have to create these sort of shimmer dummy views like PledgeShippingLocationShimmerLoadingView
, I wonder if we just set greaterThanOrEqualTo
width and height constraints on the views we want the shimmer to appear on if that would be sufficient π€
} | ||
|
||
extension ShimmerLoading where Self: UIView { | ||
private var isLoading: Bool { |
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 private isLoading
property really needed, or can we just pass it into updateAnimation
directly?
func startLoading() {
self.updateAnimation(isLoading: true)
}
} | ||
} | ||
|
||
private func allSubViews(of view: UIView) -> [UIView] { |
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.
You won't need this if you store var shimmerViews: [UIView]
instead 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.
Yip, all gone!
allSubViews(of: self) | ||
.filter { $0.shimmersWhenLoading } | ||
.forEach { view in | ||
let gradientLayer = newGradientLayer(with: view.bounds) |
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 something like shimmerGradientLayer
, shimmerAnimation
and shimmerAnimationGroup
be clearer naming for these funcs?
} | ||
|
||
extension UIView { | ||
var shimmersWhenLoading: Bool { |
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.
Also the fewer associated objects the better π
I'll look into this, it might just mean having to adjust some constraints on the view when it's loading/not loading but yeah that could work! |
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.
Lgtm! π
Going to merge this and look into possibly not using a dummy view separately π |
π² What
Adds a protocol extension
ShimmerLoading
for the purpose of adding a shimmering loading state to views.π€ Why
UIActivityIndicatorView
.π How
ShimmerLoading
protocol extension.startLoading()
andstopLoading()
functions.UIView
s are given a boolean propertyshimmersWhenLoading
which, whentrue
, means that the shimmering gradient layer is applied to them if their parent conforms toShimmerLoading
andstartLoading()
is called.UIView
s to have that boolean.PledgeShippingLocationShimmerLoadingView
example implementation.rootStackView
ofPledgeShippingLocationViewController
toShimmerLoading
and tell it which subviews to apply the shimmer to. While this would work, the shape of the views weren't quite that of the designs so I chose to create a separate view instead.π See
β Acceptance criteria
To have the view appear for longer add this to line
56
ofPledgeShippingLocationViewModel
:We will probably want to make this permanent so that the shimmer is not too brief to be jarring.