Skip to content
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

Refactoring TransitionAnimationType #137

Merged
merged 3 commits into from
Mar 30, 2016

Conversation

tbaranes
Copy link
Member

Following the discussion in #134, I tried to review the implementation of TransitionAnimationType in order to make it thiner. The current implementation is answering that needs: the enum is really smaller, and the switch is more generic / readable.

The architecture seems nice, but there are some points that need a generic solution:

  • TransitionAnimationType from string -> how to implement it?
  • Iterating on TransitionAnimationType and handles all the cases (which means with the different directions...) to update the examples. That also implicate a way to divide the final array in different section for each transition family and readable name
    For now, I just hard coded one case of SystemCube, and Fade, it's working as expected.

Any suggestions for these issues? Until now, I didn't find any nicer solution than the implementation currently used in master.

If you want to make some tests, use this branch, it's still very experimental 💭

/!\ NOT READY TO BE MERGED /!\

static func fromString(string: String) -> TransitionAnimationType? {
if String(TransitionAnimationType.Fade) == string {
return .Fade
} else if String(TransitionAnimationType.SystemCube(direction: .Left)) == string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that may not work since it is a fully qualified like SystemCube(IBAnimatable.TransitionFromDirection.Left)", we may need to do some like what we did in maskStarFromString in MaskDesignable, in IB, we may configure the string as "SystemCube(Left)"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sure. That part is really not finished 😅 I let it that way to talk that new implementation, but didn't find a nice solution yet.
In order to do as MaskDesignable, we are will need to hard code all the enums, is that a good solution?

@JakeLin
Copy link
Member

JakeLin commented Mar 28, 2016

@tbaranes nice attempt , I really like the new TransitionAnimationType, we maybe able to do similar thing for MaskType as so on.

@tbaranes
Copy link
Member Author

Here two new commits who may finalise that PR if it seems correct to everyone:

  • TransitionAnimationType: it will ask a bit more work than before, we have to add a new if for each new transition. I tried to make it generic (at least the direction). I don't like that much this generator, but don't have any better idea right now, and I'm not sure that we can make it more generic using an enum with params since we are using hard coding string from xib.
  • Example project: here again, a bit more work, one line will be add manually for each new transition, but it's worth it even if it's not generic. The interface is cleaner and more readable for the user:

simulator screen shot 29 mar 2016 20 25 28

Everything is working as expected! What do you think about that implementation? :)
Also, shouldn't we create a new release with or without that PR? The changelog is already not that bad 😆

@tbaranes tbaranes changed the title [WIP] Refactoring TransitionAnimationType Refactoring TransitionAnimationType Mar 29, 2016
@JakeLin
Copy link
Member

JakeLin commented Mar 29, 2016

Very cool implementation. I like it very much.

We may have more else if branches in static func fromString(transitionType: String) -> TransitionAnimationType? in the future to support some transition more than direction. For example, ZoomIn/Out.

I will update the wiki to mention generateTransitionTypeData when we develop new transition animations.

Please merge it when you feel is ready. 👍👍👍 Well done.

@tbaranes
Copy link
Member Author

Let's merge that first step! It's still improvable with less breaking change 😆

Could you update the CHANGELOG to explain the breaking change? Since it's a big one, I prefer let you write it to be clear. Also, if it's ok for you, we can release the IBAnimatable 2.1 👍

@tbaranes tbaranes merged commit 4292042 into master Mar 30, 2016
@tbaranes tbaranes deleted the feature/transition_animation_type_refactoring branch March 30, 2016 06:43
@JakeLin
Copy link
Member

JakeLin commented Mar 30, 2016

Cool, @tbaranes doing 2.1 now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants