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

Refactor BW::UIBarButtonItem module to a class #226

Conversation

ryansobol
Copy link
Contributor

So, I've come to a realization. I think that changing BW::UIBarButtonItem to a subclass (rather than a factory module) is a better architecture pattern for BubbleWrap's future.

My original design for BW::UIBarButtonItem was a factory module because:

  1. I was concerned that overloading +init on UIBarButtonItem would be dangerous.
  2. Someday, I wanted to explore the option of polluting UIBarButtonItem itself.
  3. I disliked the thought of typing BubbleWrap:: all the time.

However, I've since changed my mind while implementing BW::UIAlertView:

  1. lrz reminded me that overloading .new on Cocoa Touch classes is quite safe.
  2. I've given up on my dream to pollute Cocoa Touch classes because of the potential compatibility issues it creates with other 3rd party libraries.
  3. I've actually grown found of seeing BW:: in my code. :)

This patch has no behavior changes and maintains backward-compatibility. The only changes are:

  • The constructor methods now return instances of BW::UIBarButtonItem which is a subclass of UIBarButtonItem.
  • The .new constructor method replaces .build. Calling .build forwards to .new and outputs a deprecation warning to NSLog.

@clayallsopp
Copy link
Contributor

Awesome! Makes a lot of sense, brings it in line to BW::UIAlertView.

clayallsopp added a commit that referenced this pull request May 4, 2013
Refactor BW::UIBarButtonItem module to a class
@clayallsopp clayallsopp merged commit 977dd40 into rubymotion-community:master May 4, 2013
@ryansobol ryansobol deleted the refactor_ui_bar_button_item branch May 4, 2013 21:03
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