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 public intializer to PresentationDesignable #402

Merged
merged 7 commits into from Feb 22, 2017

Conversation

Projects
None yet
4 participants
@tbaranes
Member

tbaranes commented Feb 21, 2017

Follow up of #401
Since the philosophy of IBAnimatable is to use protocols for everything in order to reuse them in our own components, it's quite sad that we are blocking that usage for PresentationDesignable.

I think we just missed that case, and it's probably an implementation error.

Tom Baranes added some commits Feb 21, 2017

@tbaranes

This comment has been minimized.

Show comment
Hide comment
@tbaranes

tbaranes Feb 21, 2017

Member

@mikhailmulyar Let's continue the discussion here.

I changed a bit my mind, instead of putting everything public... I prefer abstract the setupPresenter in the protocol, which means PresentationDesignable is now usable from anywhere. AnimatableModalViewController doesn't have anymore any private properties... used to do a presentation.

Can you give it a try and let me know if that's ok for you?

Member

tbaranes commented Feb 21, 2017

@mikhailmulyar Let's continue the discussion here.

I changed a bit my mind, instead of putting everything public... I prefer abstract the setupPresenter in the protocol, which means PresentationDesignable is now usable from anywhere. AnimatableModalViewController doesn't have anymore any private properties... used to do a presentation.

Can you give it a try and let me know if that's ok for you?

@tbaranes

This comment has been minimized.

Show comment
Hide comment
@tbaranes

tbaranes Feb 21, 2017

Member

I'm even wondering if it would be worth to add a viewWillAppear to the protocol in order to abstract the dismissal animation setup in order to avoid bug cases.

Member

tbaranes commented Feb 21, 2017

I'm even wondering if it would be worth to add a viewWillAppear to the protocol in order to abstract the dismissal animation setup in order to avoid bug cases.

Tom Baranes added some commits Feb 21, 2017

@mikhailmulyar

This comment has been minimized.

Show comment
Hide comment
@mikhailmulyar

mikhailmulyar Feb 21, 2017

Looks like it will work. Gonna check it soon

mikhailmulyar commented Feb 21, 2017

Looks like it will work. Gonna check it soon

@IBAnimatableBot

This comment has been minimized.

Show comment
Hide comment
@IBAnimatableBot

IBAnimatableBot Feb 21, 2017

1 Warning
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.

Generated by 🚫 danger

IBAnimatableBot commented Feb 21, 2017

1 Warning
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.

Generated by 🚫 danger

@mikhailmulyar

This comment has been minimized.

Show comment
Hide comment
@mikhailmulyar

mikhailmulyar Feb 21, 2017

Still not able to fully use it since those errors if i'm trying to adopt PresentationDesignable as in AnimatableModalViewController.
Error: 'transitionDuration' is inaccessible due to 'internal' protection level
Error: 'presentationConfiguration' is inaccessible due to 'internal' protection level

mikhailmulyar commented Feb 21, 2017

Still not able to fully use it since those errors if i'm trying to adopt PresentationDesignable as in AnimatableModalViewController.
Error: 'transitionDuration' is inaccessible due to 'internal' protection level
Error: 'presentationConfiguration' is inaccessible due to 'internal' protection level

@tbaranes

This comment has been minimized.

Show comment
Hide comment
@tbaranes

tbaranes Feb 21, 2017

Member

This time, it compiles! I test it to make sure it was ok 😆

Member

tbaranes commented Feb 21, 2017

This time, it compiles! I test it to make sure it was ok 😆

@mikhailmulyar

This comment has been minimized.

Show comment
Hide comment
@mikhailmulyar

mikhailmulyar Feb 21, 2017

Yes, now it works. Thanks!

mikhailmulyar commented Feb 21, 2017

Yes, now it works. Thanks!

@JakeLin

👍 Good one, thanks

public extension PresentationDesignable where Self: UIViewController {
public func configurePresenter() {

This comment has been minimized.

@JakeLin

JakeLin Feb 21, 2017

Member

Great idea for having an extension method 👍

@JakeLin

JakeLin Feb 21, 2017

Member

Great idea for having an extension method 👍

Show outdated Hide outdated IBAnimatable/PresentationDesignable.swift

@tbaranes tbaranes merged commit c22ddde into master Feb 22, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tbaranes tbaranes deleted the feature/public_presentationconfiguration branch Feb 22, 2017

@tbaranes

This comment has been minimized.

Show comment
Hide comment
@tbaranes

tbaranes Feb 22, 2017

Member

@mikhailmulyar The next release will include that feature, until then, you can target master directly :)

Member

tbaranes commented Feb 22, 2017

@mikhailmulyar The next release will include that feature, until then, you can target master directly :)

@mikhailmulyar

This comment has been minimized.

Show comment
Hide comment
@mikhailmulyar

mikhailmulyar commented Feb 22, 2017

Ok. Thanks!

@JakeLin

This comment has been minimized.

Show comment
Hide comment
@JakeLin

JakeLin Feb 22, 2017

Member

Hey, @mikhailmulyar release 3.1.3 is up, you don't have to use master for the Cocoapods now, thanks.

Member

JakeLin commented Feb 22, 2017

Hey, @mikhailmulyar release 3.1.3 is up, you don't have to use master for the Cocoapods now, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment