Missing completion argument + dismiss animated flag

[gabrielPeart]

Current master sync

fix(completion): presentingViewController was been performed passing a nil competition, instead of the received argument
fix(dismiss animated): PresentrController was using a hardcoded true value. Added a dismissAnimated flag
feature(example): Added example of alert without animation and completion
feature(swiftlint): adding swiftlint
fix(swiftlint): fixing some swiftlint issues

[danlozano]

On quick inspection looks good! Definitely was missing SwiftLint and some other issues you pointed out.

As soon as I have time to look at it a little closer and test it, I will merge.

Thanks!! 👍

[danlozano]

Hey @gabrielPeart nice work!

Merged it into Develop, once I add a couple of more things I had in the pipeline it will be added to the next release. Just had a minor change, want to get your opinion on it.

You has the dissmissAnimated property on the Presentr object. The default was true. BUT every time someone did a customPresentation you re-set that property.

self.dismissAnimated = animated

Making the default of true irrelevant. I think I understand why you set it that way. Because, when someone passes in a false to the present method, you make the assumption that they don't want animation for the presentation AND the dismissal.

I removed that line of code, here's why.

Whenever someone presents a View Controller, they choose if they want to make that presentation animated or not, that's it. If inside the View Controller they want to dismiss it manually (in a manner different from the tapping of the background) they again have to choose wether to animate it or not.

So, the dissmissAnimated property, is just there because we handle the dismissal (in that case).

So, if they want the presentation to be without animation, they set it in the customPresent method. If they want the default dismissal to be without animation, then they have to set that property.

Makes sense, right?

[danlozano]

Good catch on the .FullScreen presentation type.

I originally had ignored it because I thought if they wanted to do a full screen presentation type they would just do a regular iOS view controller presentation. But, IDK, maybe it makes sense to include it here as well just to be complete. Maybe they want to have all their presentation logic consistent, instead of switching from using Presentr to using the default presentations, so they can just use the .FullScreen.

Was that kind of your reasoning as well?

[danlozano]

Oh, last thing!

Thanks for making the pull request to the Develop branch (instead of the Master like all other PR's before I added the contributing guidelines).

But I think the branch from which you do the pull request from should be a new "feature" branch, off of the Develop branch. That way we avoid some weirdness with the PR commits. I should add this to the contributing guidelines.

Thanks for your input Gabriel!

[gabrielPeart]

Hi @danlozano you are right about the dissmissAnimated point. Thanks for pointing it out.

Yes, the .FullScreen presentation type was to make it concise and consistent.

You are welcome. My initial idea was to fix a bug and I ended up adding more stuff. Mea culpa, just got excited with the current code base. For sure a new "feature" branch would be more suitable, also to be able to organise it properly and evaluate each "feature".

[gabrielPeart]

I would suggest to update the readme file as well (to explicit the new FullScreen presentation type.

[danlozano]

Hey!

Great, so we are in agreement. 👍

haha yeah that always happens. You start doing something and then you fall into the wormhole adding feature after feature. It's nice to have people contributing so much, did not expect it.

My next task is to start adding issues myself, for features I want to add. To get input, and maybe help on some of those. I'll also add some milestones for each future version to organize it a little bit more. You're welcome to add any features or suggestions!

[danlozano]

Oh yeah, I'll make sure to add both of your features (.FullScreen and DismissAnimated) to the Readme.

[danlozano]

https://github.com/IcaliaLabs/Presentr/releases/tag/0.1.8