-
Notifications
You must be signed in to change notification settings - Fork 95
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 sharing selected target view and slide in / out animation #31
Conversation
e009096
to
298e624
Compare
Codecov Report
@@ Coverage Diff @@
## v5.2 #31 +/- ##
=========================================
- Coverage 80.57% 79.48% -1.1%
=========================================
Files 163 164 +1
Lines 6385 6473 +88
=========================================
Hits 5145 5145
- Misses 1240 1328 +88
Continue to review full report at Codecov.
|
enum Design { | ||
static let height = CGFloat(79) | ||
static let bgColor = "#F7F8FA" | ||
static let borderColor = "#E6E7EA" |
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 you change the Design
as the ones in #30
- Use a
var
getter instead oflet
stored property (to make the memory a bit cleaner) - Use
UIColor(hex6:)
for colors instead ofString
(to make it more efficient) - Add all design style related things here, like
borderWidth
,backgroundColor
, etc.
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. let me modified.
@objc | ||
func foo() { | ||
selectedTargetView.mode = (selectedTargetView.mode == .show) ? .hide : .show | ||
selectedTargetView.updateLayout(withAnimated: true) |
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 seems that every mode
changing is bound to a call of updateLayout(withAnimated:)
. Can we combine these two, hide both mode
and updateLayout
as private, then provide an easier API like setMode(_ mode: Mode, animated: 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.
👍
|
||
var mode = Mode.hide | ||
|
||
func updateLayout(withAnimated animated: 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.
The API name is not common. Maybe updateLayout(animated flag: Bool)
would be more swifty. See the similar one from Apple.
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.
Or you can call it "withAnimation:", but that indicates that you want to pass an animation
object which can define what exactly the animation is. For a Bool
, I guess it is proper to use xxx(animated:)
yAnchor = view.safeAreaLayoutGuide.bottomAnchor | ||
} else { | ||
yAnchor = bottomLayoutGuide.topAnchor | ||
} |
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.
static let borderColor = "#E6E7EA" | ||
} | ||
|
||
private var slideAnimationViewTopConstraint: NSLayoutConstraint? |
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.
Since slideAnimationViewTopConstraint
would be always there unless a programmer error happens. So let's change it to NSLayoutConstraint!
to make the failure explicitly.
… to make the memory a bit cleaner, applied UIColor(hex6:)
self.alpha = alpha | ||
self.layoutIfNeeded() | ||
} | ||
} |
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 guess if animated == false
, the alpha
is not getting updated now. Maybe we also need the case for non-animated
?
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.
And I suggest also call layoutIfNeeded()
in the animated == false
statement, to trigger an immediate layout. Otherwise, any code depends on the layout would not work until the next main run loop, which causes confusion.
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.
Indeed, I missed the alpha setup in no animation case.
Agree. the layoutIfNeeded
in animated == false
will be more clear.
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.
Review is done. Please check the comments before it can be merged.
About
Note