-
Notifications
You must be signed in to change notification settings - Fork 170
Add Animation Completion Parameter #60
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
base: master
Are you sure you want to change the base?
Conversation
| let aboveCell = cellAbove(cell: cell) | ||
|
|
||
| let completion: (Bool) -> Void = { [weak self] _ in | ||
| let completion: (Bool) -> Void = { [weak self] flag in |
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 name of the handler conflicts with the argument to the method. Perhaps rename the local variable to completionHandler to disambiguate. Also the argument to the function might be better called finished rather than flag.
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.
thank you for your feedback!
i applied all feedbacks. could you check one more time?
| if let aboveCell = aboveCell { | ||
| self.updateSeparatorVisibility(forCell: aboveCell) | ||
| } | ||
| completion?(flag) |
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.
Insert one blank line above this line.
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 did
| UIView.animate(withDuration: 0.3) { | ||
| UIView.animate(withDuration: 0.3, animations: { | ||
| cell.alpha = 1 | ||
| }) { flag in |
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 is a little confusing, perhaps we could split this handler out to a separate completionHandler above, like with the removeCell method?
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.
remove closure block, just input the argument
| /// If `animated` is `true`, the insertions are animated. | ||
| open func prependRows(_ rows: [UIView], animated: Bool = false) { | ||
| rows.reversed().forEach { prependRow($0, animated: animated) } | ||
| open func prependRows(_ rows: [UIView], animated: Bool = false, 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.
please wrap these function signatures on to multiple lines
open func prependRows(
_ rows: [UIView],
animated: Bool = false,
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.
i broke the line of function name that longer then 110 characters.
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.
however, here is my break line style.
open func prependRows(_ rows: [UIView],
animated: Bool = false,
completion: ((Bool) -> Void)? = nil) {
// code is here
}
if you don't like this style please commit the additional feedback. i will apply as soon as possible.
| open func addRow(_ row: UIView, animated: Bool = false) { | ||
| insertCell(withContentView: row, atIndex: stackView.arrangedSubviews.count, animated: animated) | ||
| open func addRow(_ row: UIView, animated: Bool = false, completion: ((Bool) -> Void)? = nil) { | ||
| insertCell(withContentView: row, atIndex: stackView.arrangedSubviews.count, animated: animated, completion: completion) |
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.
please wrap multiple statements onto separate lines
insertCell(
withContentView: row,
atIndex: stackView.arrangedSubviews.count,
animated: animated,
completion: completion)
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.
same as above comment
- insert blank line - change argument, variable name - truncate line what function name that parameters length longer than 110 characters
| open func addRow(_ row: UIView, animated: Bool = false) { | ||
| insertCell(withContentView: row, atIndex: stackView.arrangedSubviews.count, animated: animated) | ||
| open func addRow(_ row: UIView, animated: Bool = false, completion: ((Bool) -> Void)? = nil) { | ||
| insertCell(withContentView: row, |
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 parameter should start on the next line:
insertCell(
withContentView: row,
atIndex: stackView.arrangedSubviews.count,
animated: animated,
completion: completion)if you could apply this style to the other callsites that would be great!
|
Hi @blick9, thanks for doing this work - on further consideration it feels like we might want to wait until a decision is made on #12 as it seems the implementation of custom row animations could have a big impact on this API. We'll take a look at PR#12 and get back to you on this one. Sorry for the delay on this! |
|
@apang42 @marlimox @aloestackview-admin Hello, it's already been a year. do you have no plans to update any more? |
hello, I need a callback for animation when I use it by my case.
but, It was not implemented, so I made a request like this.
I hope to be approved