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

Uiview fade animate #299

Merged
merged 8 commits into from Nov 18, 2016
Merged

Conversation

vilapuigvila
Copy link
Contributor

Checklist

  • New Extension
  • New Test
  • Changed more than one extension, but all changes are related
  • Trivial change (doesn't require changelog)

@EZSwiftExtensionsBot
Copy link

EZSwiftExtensionsBot commented Nov 5, 2016

2 Messages
📖 Executed 145 tests, with 0 failures (0 unexpected) in 4.742 (5.035) seconds
📖 Executed 136 tests, with 0 failures (0 unexpected) in 2.407 (2.835) seconds

Generated by 🚫 danger

@codecov-io
Copy link

codecov-io commented Nov 5, 2016

Current coverage is 36.60% (diff: 100%)

Merging #299 into master will increase coverage by 1.64%

@@             master       #299   diff @@
==========================================
  Files            46         46          
  Lines          2042       2076    +34   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            714        760    +46   
+ Misses         1328       1316    -12   
  Partials          0          0          

Powered by Codecov. Last update 704a823...e5fbe9f

///EZSE: Fade in with duration, delay and completion block.
public func fadeIn(_ duration:TimeInterval?, delay _delay:TimeInterval?, completion: ((Bool) -> Void)? = nil) {
UIView.animate(withDuration: duration ?? UIViewFadeDuration, delay: _delay ?? 0.0, options: UIViewAnimationOptions(rawValue: UInt(0)), animations: {
self.alpha = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

self.alpha = 1.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You would say fadeIn = alpha 1.0, and fadeOut = alpha 0.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right!

if let completion = completion {
completion(success)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better that completion set to UIView.animate func directly.

}, completion: completion)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right!


extension UIView {
///EZSE: Fade in with duration, delay and completion block.
public func fadeIn(_ duration:TimeInterval?, delay _delay:TimeInterval?, completion: ((Bool) -> Void)? = nil) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to use Default Parameter Values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my case we can use the function like this aView.fadeIn(nil, delay: 1.0, completion: nil) the nil values will be Default Parameter Values, but we lost the aView.fadeIn() definition. However I'll re-write function as you think.
Thanks.

Easily to fade in/out or a specific alpha value with completion blocks:

``` swift
view.fadeInWithDuration(0.5, delay: 1.0) { (success) in
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please describe the correct usage?

@furuyan
Copy link
Contributor

furuyan commented Nov 6, 2016

Could you fix swiftlint warnings?

@lfarah
Copy link
Collaborator

lfarah commented Nov 6, 2016

Thanks for contributing, @vilapuigvila! 😃 👍

completion(success)
}
}
public func fadeIn(_ duration:TimeInterval? = UIViewFadeDuration, delay _delay:TimeInterval? = 0.0, completion: ((Bool) -> Void)? = nil) {
Copy link
Contributor

Choose a reason for hiding this comment

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

duration and delay should not be Optional.
Because, if user set nil to duration or delay, application crashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glups.. eventually I've keep both, I think will be more useful. We can implement view.fadeIn() or view.fadeIn(nil, delay: 1.5, completion: nil) and much more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ummm.. I think that omitting the unnecessary parameters is enough for users.
and, To match implementation with other methods is important.

What do you guys think?

@furuyan
Copy link
Contributor

furuyan commented Nov 7, 2016

@vilapuigvila Thanks for fixing and good extensions 😄

@lfarah
Copy link
Collaborator

lfarah commented Nov 10, 2016

@vilapuigvila can you please add tests for this one too?

@vilapuigvila
Copy link
Contributor Author

@lfarah Of course, this week I don't have much time, I'm very busy, I'll try this night.

@Esqarrouth
Copy link
Owner

@lfarah Since we're redoing the readme, is it wise to add the new examples to the old readme?

@lfarah
Copy link
Collaborator

lfarah commented Nov 16, 2016

@goktugyil we're only adding important ones and not every single one

@lfarah
Copy link
Collaborator

lfarah commented Nov 16, 2016

@goktugyil I don't think we should add new examples to old readme

func testFadeOut() {

let view = UIView()
view.alpha = 0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

This value should not be 0.

@Esqarrouth
Copy link
Owner

@lfarah This commit has an entry to the readme

@lfarah
Copy link
Collaborator

lfarah commented Nov 16, 2016

@vilapuigvila can you please remove it?

@vilapuigvila
Copy link
Contributor Author

Removed.

@lfarah lfarah merged commit 6110a95 into Esqarrouth:master Nov 18, 2016
@lfarah
Copy link
Collaborator

lfarah commented Nov 18, 2016

Thanks for all the patience, @vilapuigvila and thanks to @furuyan for the code reviews!

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

6 participants