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

Add UIActionSheet wrapper? #367

Open
hollingberry opened this issue Apr 17, 2014 · 24 comments
Open

Add UIActionSheet wrapper? #367

hollingberry opened this issue Apr 17, 2014 · 24 comments

Comments

@hollingberry
Copy link

An example:

For more information, see the UIActionSheet Class Reference.

Action sheets are used really often, and have an ugly API, so it seems like a good spot for BubbleWrap to step in?

@hollingberry
Copy link
Author

I could try doing a PR...

@clayallsopp
Copy link
Contributor

This could be cool - would love a PR for it. What would your API look like?

@hollingberry
Copy link
Author

Maybe something like

BW::UIActionSheet.new(
  origin: UIBarButtonItem.alloc.init, # ignored on an iPhone
  buttons: [
    {
      title: "Cancel",
      type: :cancel
    },
    {
      title: "Delete Note",
      type: :destructive,
      action: :delete_note
    },
    {
      title: "Something Else",
      action: :do_something_else
    }
  ],
  animated: true
)

I like the simplicity of having all the buttons in one hash, but with this API, there would have to be some kind of check to make sure there is only one :cancel button and one :destructive button. What do you think?

@clayallsopp
Copy link
Contributor

Seems pretty reasonable, and definitely helpful.

Are the :actions method names? what do you think of something like:

as = BW::UIActionSheet.new()
as.on(:delete_note) do
end
as.on(:do_something_else) do
end

cc @colinta @markrickert

@hollingberry
Copy link
Author

I like that. What do you think about also being able to, as an alternative, pass a lambda?

@markrickert
Copy link
Collaborator

I like the direction this conversation is going. I'll be watching :)

@supermarin
Copy link
Contributor

@clayallsopp how would that link to titles?

@hollingberry
Copy link
Author

@supermarin I think the idea was that you would define the action as a symbol, and then reference the symbol in the event callback. For example, above, the action is defined as action: :delete_note, meaning that you can respond to it as:

as.on(:delete_note) do
end

@colinta
Copy link
Contributor

colinta commented Apr 18, 2014

Yeah I really like that :title / :action combo. Works great with the blocks!

@hollingberry
Copy link
Author

Let's also add title: and style: properties to the options hash. title: is a string, and style: is a symbol that corresponds to a constant (this can be one of :automatic, :default, :black_translucent, or :black_opaque.

@colinta
Copy link
Contributor

colinta commented Apr 18, 2014

I was playing around with what this would look like in SugarCube's existing UIActionSheet wrapper, and came up with this (alert view and action sheet examples). This would save you from the on(...) methods, and you could return the actual UIActionSheet object...

BW::UIAlertView.new('Confirm action!',
  message: 'Are you sure you want to do this?',
  buttons: {
    cancel: 'No!',  # special treatment
    success: 'Sure!',
    unsure: 'Hmmm',
  }) do |button|
  # button will be :cancel, :success or :unsure
end

BW::UIActionSheet.new('Well, how bout it?',
  buttons: {
    help: 'Tell me more',
    cancel: 'Cancel',  # special treatment
    destructive: 'Kill it with fire!',  # special treatment
  }) do |button|
  # button is :cancel, :destructive or :help
end

@colinta
Copy link
Contributor

colinta commented Apr 18, 2014

This would have the advantage of having the two interfaces identical...

haha, hmm, is that an advantage or confusing? 😕

@hollingberry
Copy link
Author

Maybe. What about UIActionSheet.show instead of UIActionSheet.alert.

@colinta
Copy link
Contributor

colinta commented Apr 18, 2014

Oh I fixed that, it should be new if it's going to match BW::UIAlertView - I forgot that already exists, though, so we should have BW::UIActionSheet match that interface.

@hollingberry
Copy link
Author

I feel like the problem about BW::UIAlertView and BW::UIActionSheet having the same API is that unlike UIAlertView, UIActionSheet doesn't require a title (in fact, it usually isn't used with one).

@colinta
Copy link
Contributor

colinta commented Apr 18, 2014

That's not an issue; options[:title] becomes optional in UIActionSheet

@colinta
Copy link
Contributor

colinta commented Apr 18, 2014

I'd like to rescind the code above - I forgot that bubble-wrap already added a wrapper for UIAlertView, and that's what we should model BW::UIActionSheet after.

All I meant to propose was a syntax for the buttons that would obviate the need for an on() method, but since BW::UIActionSheet would logically return a subclass of ::UIActionSheet it's really not an issue.

So, basically, I'm just adding a lot of noise to this conversation and it should be largely ignored! Sorry @ALL! :-)

@hollingberry
Copy link
Author

I think that the reason it makes sense to have title as the first parameter of BW::UIAlertView.new and not of BW::UIActionSheet.new is that often, unlike action sheets, you will be making an alert with only a title.

@colinta
Copy link
Contributor

colinta commented Apr 18, 2014

Sounds reasonable, but BW::UIAlertView doesn't work that way, you assign it a title only by passing BW::UIAlertView.new(title: 'this is the only way')

@hollingberry
Copy link
Author

Oops, I was getting confused with App.alert.

@colinta
Copy link
Contributor

colinta commented Apr 18, 2014

Oh and I didn't even know about that one! Different APIs for App.alert and BW::UIAlertView.new, that's a bummer. I like the title as first arg thing.

I think a similar thing could be done here - if App.sheet is implemented, it could accept a hash in first or last position, so the title could be the first arg (which makes sense) or it could be skipped.

@hollingberry
Copy link
Author

Combining some of the ideas from above, and just to put another idea out there, we could get rid of the first parameter title and do something like this:

BW::UIActionSheet.new({
  title: 'Some title',
  animated: true,
  origin: UIBarButtonItem.alloc.init, # ignored on iPhone
  style: :black_translucent,
  buttons: {
    help: 'Tell me more',
    cancel: 'Cancel',  # special treatment
    destructive: 'Kill it with fire!',  # special treatment
  }) do |button|
  # button is :help, :cancel, or :destructive
end

Then again, I like having the possibility of using a lambda directly in the buttons hash because it eliminates the need for a case statement.

@GantMan
Copy link
Contributor

GantMan commented Apr 28, 2014

anyone working on this?

@twe4ked
Copy link

twe4ked commented Nov 13, 2014

Looks like these classes have been replaced with UIAlertController.

This class [UIAlertController] replaces the UIActionSheet and UIAlertView classes for displaying alerts.

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

No branches or pull requests

7 participants