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

Several improvements #5

Merged
merged 2 commits into from
Aug 30, 2013
Merged

Several improvements #5

merged 2 commits into from
Aug 30, 2013

Conversation

JonasGessner
Copy link

• Improved tap to dismiss
• Moved private stuff into separate header
• Improved customizablility for ALAertBannerStyle
• Removed experimental iOS 4.3 support (-[UIColor darkerColor] requires
iOS 5)
• General improvements and fixes

• Improved tap to dismiss
• Moved private stuff into separate header
• Improved customizablility for ALAertBannerStyle
• Removed experimental iSO 4.3 support (-[UIColor darkerColor] requires
iOS 5)
• General improvements and fixes
@lobianco
Copy link
Owner

Hi @JonasGessner. Can you update the demo project so it builds successfully so I can review the changes a little easier?

@JonasGessner
Copy link
Author

Sorry, I actually did that but in the wrong project :P Hold on.

@lobianco
Copy link
Owner

Thanks, I'll take a look at this soon.

@lobianco
Copy link
Owner

Good call on [UIColor darkerColor].

You've definitely made a ton of changes. Which do you feel strongest about, and what's your reasoning?

I'm not comfortable merging such a large amount of changes. It'd be easier for me if you submitted separate pull requests, maybe one or two for enhancements and one for cleanup. There's definitely a few things in there that I wouldn't mind bringing in.

@JonasGessner
Copy link
Author

The new tap to dismiss is probably what I like most of the changes. Using the banners with the new tap to dismiss feels really great!

I had to rename the properties of ALAlertBanner because UIControl (its new superclass) implements the state property alteady so I renamed the state property and to keep everything balanced I also changed the other properties to fit in.

The private header file doesn't need too much explanation I guess, it just cleans everything up a bit ;)

The new way o customizing the style is neat but for me its not really a must. That would be the only thing where I'd be ok to put it back to the original state. On the other hand its a great feature for customizing the banner's appearance so It could also be really helpful.

@lobianco
Copy link
Owner

Thanks for your contributions, they are much appreciated. The style creation is a very nice idea. I'm going to fiddle around with it a little more before merging.

+1 on the private header.

I don't think I want to incorporate UIControl behavior at this time. It just feels too... interactive. I might give it some more thought down the road though.

@lobianco
Copy link
Owner

If you want to revert the UIControl changes and property names I'll merge the PR. Or I can merge locally and make the changes myself if you prefer.

The only thing that bothers me with ALAlertBannerStyleMake is that the #defined constants (such as kALAlertBannerStyleSuccess) break the naming consistency with the other two typedef'ed properties (state and position). It's fastidious, I know, but do you have any propositions on how to deal with that?

@lobianco lobianco merged commit ab3a5bc into lobianco:master Aug 30, 2013
@lobianco
Copy link
Owner

Merged locally and made a few changes. I don't think the makeStyle functionality is quite ready yet. I'd like to find a better way to integrate it without having to force the user to use a #define if they want to use one of the default styles. If you have any suggestions I'm open to them.

Thanks again!

@JonasGessner
Copy link
Author

Yeah it is a bit weird to use but the defines definitely make it a lot easier! The defines could just be named AlAlertBannerStyle... to match the names of the other properties.

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.

2 participants