Add CustomAnimationConvertible #12

ParkGwangBeom · 2018-10-29T06:27:50Z

Hi. I added the animation feature that I mentioned in the issue. (#10)

The gif below is a simple example.
If you use the newly added Animation feature, you will be able to customize the animation to be more extensible on its own line.

tell me your opinions.
Thank you.

ParkGwangBeom · 2018-11-01T23:18:52Z

@marlimox Take care. Please take a look at this pr. If you do not like the approach to new features, please give me another comment! :)

marlimox

Hi @ParkGwangBeom, I am so sorry for the very slow review of this PR! I apologize 😔 I have been busy traveling, but I have finally had a moment to review it. It looks great, thank you for this contribution!

I agree that this will be useful functionality to have and it shouldn’t complicate the implemention of AloeStackView too much. I have some comments that I hope might simplify the implementation further. Please take a look when you get the chance and let me know what you think.

Thanks again for your great contributions to AloeStackView! You have contributed some really great work 😊

Example/AloeStackViewExample/Views/AnimatableDescRowView.swift

Example/AloeStackViewExample/ViewControllers/MainViewController.swift

Example/AloeStackViewExample/Views/AnimatableDescRowView.swift

Sources/AloeStackView/Protocols/CustomAnimationConvertible.swift

Example/AloeStackViewExample/Views/AnimatableRowLabel.swift

marlimox · 2018-11-11T23:22:32Z

Sources/AloeStackView/Animation/AnimationCoordinator.swift

+/**
+ * An object that animates when the Row is inserted and deleted from the AloeStackView.
+ */
+public class AnimationCoordinator {


I would like to simplify this implementation by potentially changing how this works. Rather than passing an AnimationCoordinator into the row view, what if instead the custom animating protocol had methods such as:

func animateInsert() func animateRemove()

These methods would be called from inside the animation block when the row insertion or removal happens. The view can make any changes it wants inside these methods and the changes will be animated.

Additionally, we could add some methods to the custom animating protocol to allow for the initial state to be configured before the animation, and to perform actions after the animation completes, for example:

func willAnimateInsert() func didAnimateInsert() func willAnimateRemove() func didAnimateRemove()

You could then implement custom animations by doing something like:

func willAnimateInsert() { transform = CGAffineTransform(translationX: -100, y: 0) } func animateInsert() { transform = .identity }

I think this could be a lot simpler and clearer. Let me know what you think of this approach.

Thank you for your feedback.
I also implemented it early on through the above approach.
If you implement it in the above way, there will be an advantage that the function name makes meaning clear.
But there are too many functions inside the protocol.
So to simplify this, I thought about the viewWillTransition function which is the api of UIVIewController.
Please tell me your thoughts on my opinion.
Thank you!

Yes you’re right, it does create too many functions in the protocol.

My plan to address that was to add a protocol extension with empty definitions for every function. That way views only need to implement the specific functions they care about and don’t need to implement every function.

For example:

extension CustomAnimating { func willAnimateInsert() { } func animateInsert() { } func didAnimateInsert() { } ... }

I think this will give the best of both worlds. It also has the advantage that we can add more custom animation methods in the future, for example we can add methods for customizing show and hide animations, without having one big animation method in every view.

I also updated the names of their methods in my previous comments to better match the UIKit will/did naming convention.

Let me know what you think!

Listening to you, I think it makes sense! 👍

protocol CustomAnimating { func insertAnimationWillBegin() func animateInsert() func insertAnimationDidEnd(_ completed: Bool) ... }

What do you think about naming such a thing?
I want to hear your thoughts.

Yes, great suggestion! I like it much better 😄

Perhaps it might be easier to understand if we put the protocol methods in this order:

protocol CustomAnimating { /// ... func animateInsert() /// ... func insertAnimationWillBegin() /// ... func insertAnimationDidEnd(_ finished: Bool) ... }

That way people will see the most important method in the protocol first (the one they need to implement to perform the custom animation).

Also, perhaps the argument to insertAnimationDidEnd should be named finished rather than completed so that it matches the name UIKit uses in animate(withDuration:animations:completion:) (the completion callback takes an argument named finished).

Let me know your thoughts!

I agree with your thoughts. 👍

ParkGwangBeom · 2018-11-12T04:08:19Z

@marlimox Thank you for your detailed review. We have edited except for the last review.
The fix for CustomAnimating protocol would like to hear more from you!
Thank you.

marlimox

Looks good. I’ve added a few more comments, please take a look.

marlimox · 2018-11-12T11:04:46Z

Example/AloeStackViewExample/ViewControllers/MainViewController.swift

-    label.numberOfLines = 0
-    label.text = "This simple app shows some ways you can use AloeStackView to lay out a screen in your app."
-    stackView.addRow(label)
+    let descriptionRow = TitleCaptionRow(title: "This simple app shows some ways you can use AloeStackView to lay out a screen in your app.")


👌

Thanks for fixing this up!

marlimox · 2018-11-12T11:07:20Z

Example/AloeStackViewExample/Views/TitleCaptionRow.swift

+
+  // MARK: Private
+
+  private let title: String


You shouldn’t need to store these, just set the text properties on the labels directly.

marlimox · 2018-11-12T11:08:28Z

Example/AloeStackViewExample/Views/TitleCaptionRow.swift

+  }
+
+  private func setUpCaptionLabel() {
+    guard let captionText = captionText else { return }


guard captionLabel.text != nil else { return }

marlimox · 2018-11-12T11:08:59Z

Example/AloeStackViewExample/Views/TitleCaptionRow.swift

+
+  // MARK: Lifecycle
+
+  public init(title: String, captionText: String? = nil) {


Naming: titleText

marlimox · 2018-11-12T11:09:29Z

Example/AloeStackViewExample/Views/TitleCaptionRow.swift

+    self.title = title
+    self.captionText = captionText
+    super.init(frame: .zero)
+    setUpSelf()


Call this from setUpViews.

marlimox · 2018-11-12T11:14:03Z

Example/AloeStackViewExample/ViewControllers/MainViewController.swift

@@ -41,16 +41,15 @@ public class MainViewController: AloeStackViewController {
    setUpDescriptionRow()
    setUpSwitchRow()
    setUpHiddenRows()
+    setUpCustomAnimatingDescRow()


Naming: setUpCustomAnimatingDescriptionRow.

marlimox · 2018-11-12T11:14:33Z

Example/AloeStackViewExample/ViewControllers/MainViewController.swift

@@ -41,16 +41,15 @@ public class MainViewController: AloeStackViewController {
    setUpDescriptionRow()
    setUpSwitchRow()
    setUpHiddenRows()
+    setUpCustomAnimatingDescRow()
+    setUpHiddenAnimatableRow()


Naming: setUpCustomAnimatingRow.

marlimox · 2018-11-12T11:15:23Z

Example/AloeStackViewExample/ViewControllers/MainViewController.swift

+    stackView.addRow(animatableRow)
+    stackView.setTapHandler(forRow: animatableRow) { [weak self] _ in
+      guard let `self` = self else { return }
+        let isHidden = self.stackView.isRowHidden(self.animatableHiddenLabel)


Style: indent back two spaces.

marlimox · 2018-11-12T11:19:03Z

Example/AloeStackViewExample/ViewControllers/MainViewController.swift

+    }
+  }
+
+  private let animatableHiddenLabel = CustomAnimatingLabel()


Naming: customAnimatingLabel.

marlimox · 2018-11-12T11:20:43Z

Sources/AloeStackView/Protocols/CustomAnimationConvertible.swift

@@ -0,0 +1,27 @@
+// Created by gwangbeom on 10/29/18.


Rename this file to CustomAnimating.swift.

ParkGwangBeom · 2018-11-13T04:49:27Z

@marlimox We reflected your review.
I would like to talk more about the function of CustomAnimating. :)

ParkGwangBeom · 2018-11-28T03:52:05Z

@marlimox
Hi. The company was busy for a while, so I could not get additional PR.
I will post additional PR soon.
Thank you.

marlimox · 2018-11-28T22:00:34Z

No problem @ParkGwangBeom! Whenever you get the time 😄

gbrhaz · 2019-01-29T15:08:12Z

@ParkGwangBeom @marlimox Insert animations are also a requirement for me - what's the current status of this PR? Is there anything I/we/anyone else can do that can help it along?

ParkGwangBeom · 2019-03-16T05:08:18Z

@marlimox @gbrhaz
Sorry. The busy project schedule is over. I will finish the remaining pr during next week. :)

ParkGwangBeom · 2019-03-17T10:00:02Z

@marlimox Rebases were performed, and additional reviews were reflected. :)

ParkGwangBeom · 2019-07-05T08:59:06Z

@apang42 Hello, When will this pull request become a merge?

apang42 · 2019-07-18T17:58:47Z

Hey @ParkGwangBeom so sorry for the delay on this issue, we'll address this as soon as we can!

ParkGwangBeom · 2019-10-19T15:08:10Z

@apang42 I resolved the conflict.

ParkGwangBeom · 2020-02-18T15:43:42Z

@apang42 @marlimox @aloestackview-admin
When will this pull request be merged?

ParkGwangBeom changed the title ~~Feature/animatable~~ Add CustomAnimationConvertible Oct 29, 2018

ParkGwangBeom mentioned this pull request Nov 3, 2018

Suggested additional features #10

Open

marlimox self-requested a review November 3, 2018 23:17

marlimox reviewed Nov 11, 2018

View reviewed changes

marlimox reviewed Nov 12, 2018

View reviewed changes

ParkGwangBeom force-pushed the feature/animatable branch from 559c184 to 783700a Compare March 17, 2019 09:58

apang42 mentioned this pull request Aug 29, 2019

Add Animation Completion Parameter #60

Open

ArLupin added 8 commits October 19, 2019 23:59

Add CustomAnimationConvertible

2ccfa47

Add example

ac252a6

Sample source code review reflected

99633bb

Reflected reviews

6f1960f

Modify the code style to suit

b1fdbdd

Reflected reviews

10ac52a

Reflected reviews

b5bb1a2

Fix style

d910c99

ParkGwangBeom force-pushed the feature/animatable branch from f9277bd to d910c99 Compare October 19, 2019 15:07

fix code style

b6592f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CustomAnimationConvertible #12

Add CustomAnimationConvertible #12

ParkGwangBeom commented Oct 29, 2018 •

edited

Loading

ParkGwangBeom commented Nov 1, 2018

marlimox left a comment

marlimox Nov 11, 2018 •

edited

Loading

ParkGwangBeom Nov 12, 2018

marlimox Nov 12, 2018 •

edited

Loading

ParkGwangBeom Nov 13, 2018

marlimox Nov 13, 2018 •

edited

Loading

ParkGwangBeom Nov 14, 2018

ParkGwangBeom commented Nov 12, 2018

marlimox left a comment

marlimox Nov 12, 2018

marlimox Nov 12, 2018

marlimox Nov 12, 2018

marlimox Nov 12, 2018

marlimox Nov 12, 2018

marlimox Nov 12, 2018

marlimox Nov 12, 2018

marlimox Nov 12, 2018

marlimox Nov 12, 2018

marlimox Nov 12, 2018

ParkGwangBeom commented Nov 13, 2018

ParkGwangBeom commented Nov 28, 2018

marlimox commented Nov 28, 2018

gbrhaz commented Jan 29, 2019

ParkGwangBeom commented Mar 16, 2019

ParkGwangBeom commented Mar 17, 2019

ParkGwangBeom commented Jul 5, 2019

apang42 commented Jul 18, 2019

ParkGwangBeom commented Oct 19, 2019

ParkGwangBeom commented Feb 18, 2020


		// MARK: Lifecycle

		public init(title: String, captionText: String? = nil) {

Add CustomAnimationConvertible #12

Are you sure you want to change the base?

Add CustomAnimationConvertible #12

Conversation

ParkGwangBeom commented Oct 29, 2018 • edited Loading

ParkGwangBeom commented Nov 1, 2018

marlimox left a comment

Choose a reason for hiding this comment

marlimox Nov 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marlimox Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marlimox Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParkGwangBeom commented Nov 12, 2018

marlimox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParkGwangBeom commented Nov 13, 2018

ParkGwangBeom commented Nov 28, 2018

marlimox commented Nov 28, 2018

gbrhaz commented Jan 29, 2019

ParkGwangBeom commented Mar 16, 2019

ParkGwangBeom commented Mar 17, 2019

ParkGwangBeom commented Jul 5, 2019

apang42 commented Jul 18, 2019

ParkGwangBeom commented Oct 19, 2019

ParkGwangBeom commented Feb 18, 2020

ParkGwangBeom commented Oct 29, 2018 •

edited

Loading

marlimox Nov 11, 2018 •

edited

Loading

marlimox Nov 12, 2018 •

edited

Loading

marlimox Nov 13, 2018 •

edited

Loading