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

Remove :confirm in favor of :data => { :confirm => 'Text' } option #6614

Merged
merged 1 commit into from Jul 18, 2012

Conversation

carlosgaldino
Copy link
Contributor

Just like 683fc4d this removes the :confirm option.

/cc @rafaelfranca

The PR adding the deprecation is #6613

@rafaelfranca
Copy link
Member

ditto #6613 (comment)

@ghost ghost assigned rafaelfranca Jun 3, 2012
@carlosgaldino
Copy link
Contributor Author

Updated

@@ -10,6 +10,5 @@

<p>
<%= link_to 'Destroy Comment', [comment.post, comment],
:confirm => 'Are you sure?',
:method => :delete %>
:method => :delete, :data => { :confirm => 'Are you sure?' } %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to maintain the indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@carlosantoniodasilva
Copy link
Member

@carlosgaldino mate, can you rebase this from current master (it's not applying cleanly anymore), and fix what @rafaelfranca asked, so we can get this merged? Thanks!

@carlosgaldino
Copy link
Contributor Author

@carlosantoniodasilva Ok, I'll probably do this today or tomorrow. Well, I'll let you know when it's done. Thanks for the comments.

@carlosgaldino
Copy link
Contributor Author

@carlosantoniodasilva Updated. Please take a look and let me know if I there's something else to do.

@carlosantoniodasilva
Copy link
Member

@carlosgaldino thanks!

It looks ok for me to merge, but I have a related questions: if we're removing all docs related to :confirm, how's the user supposed to know he can use the data attributes, such as :confirm and disable_with? Just something I started thinking about.

/cc @rafaelfranca

@carlosgaldino
Copy link
Contributor Author

@carlosantoniodasilva There are some examples like this but do you think it's better to add something like:

You can use the HTML5 custom data attributes by passing the values inside a data hash. For example: data => { :something => "bla bla bla", :confirm => "You sure?" }

It doesn't need to be exactly as I typed above but you get the idea.

@carlosantoniodasilva
Copy link
Member

I see the examples, but I'm afraid there's no explanation anymore about what they do, since we're removing the related docs. I'm just unsure about how to approach that.

Summoning @vijaydev to help us with the doc related question :)

@vijaydev
Copy link
Member

vijaydev commented Jul 4, 2012

We need to add it under the list of options, for eg, here and explain it thus:

"This option can be used to add custom data attributes :data => { :confirm => "You sure?" }"

This should be done wherever the :confirm option was removed in the list of options.

@carlosgaldino
Copy link
Contributor Author

I updated the comments showing that there's a data option.

@@ -1152,13 +1152,12 @@ together.

Here we're using +link_to+ in a different way. We wrap the
+:action+ and +:id+ attributes in a hash so that we can pass those two keys in
first as one argument, and then the final two keys as another argument. The +:method+ and +:confirm+
first as one argument, and then the final two keys as another argument.The +:method+ and +:data-confirm+
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:data-confirm is invalid syntax

This applies to the following helpers:
`button_to`
`button_tag`
`image_submit_tag`
`link_to`
`submit_tag`
rafaelfranca added a commit that referenced this pull request Jul 18, 2012
Remove `:confirm` in favor of `:data => { :confirm => 'Text' }` option
@rafaelfranca rafaelfranca merged commit 3507216 into rails:master Jul 18, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants