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 docs for data-turbo-method and data-turbo-confirm for link_to #47505

Merged

Conversation

julianfssen
Copy link
Contributor

Motivation / Background

Rails 7 ships with Turbo enabled by default. Instead of using data-method and data-confirm, Turbo now uses data-turbo-method and data-turbo-confirm to perform a link visit with the specified HTTP method and show confirmation dialog respectively.

The deprecated legacy options are documented but the new options are not.

Detail

This commit documents the data-turbo-method and data-turbo-confirm for the link_to method. The button_to documentation has also been updated to reference the new link_to options.

Additional information

There is an argument that this change is unrelated to Rails, since it is a Turbo-specific behavior.

However, I feel it needs to be documented because:

  1. Turbo is enabled by default so people who were used to the legacy data-method and data-confirm may not be aware of it when upgrading or starting a new Rails 7 project.
  2. The new options are referenced in the getting started guide, so it makes sense to reference it in the API documentation as well.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the actionview label Feb 26, 2023
@ghiculescu
Copy link
Member

These examples are helpful, but I think it would also be good to link to the Turbo Handbook.

# link_to "Delete profile", @profile, data: { turbo_method: :delete }
# # => <a href="/profiles/1" data-turbo-method="delete">Delete profile</a>
#
# Since forms cannot appear inside another form, +button_to+ <i>cannot</i> be used here as it returns a form.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Doesn't data-turbo-method also generate a form at runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question, @ghiculescu !

data-turbo-method does generate a form at runtime. But, it appends the generated form as a child of document.body instead of it returning it directly like button_to, which is a neat shortcut to make non-GET requests work in forms.

The Turbo handbook does mention this:

The link will get converted into a hidden form next to the a element in the DOM. This means that the link can’t appear inside another form, as you can’t have nested forms.

I'm not entirely sure if that's accurate since this type of link works in my testing. The source code also suggests that the form is appended to document.body rather than the link where data-turbo-method is called.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure if that's accurate since this type of link works in my testing. The source code also suggests that the form is appended to document.body rather than the link where data-turbo-method is called.

Should probalby update the Turbo handbook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, @ghiculescu ! I will create a PR shortly for updating the Turbo handbook regarding data-turbo-method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

# A common use case is to insert links to perform update or delete operations (i.e. non-<tt>GET</tt> requests) in a form.
# For example, you might use +link_to+ in an edit form to delete a record:
#
# link_to "Delete profile", @profile, data: { turbo_method: :delete }
Copy link
Member

Choose a reason for hiding this comment

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

This example should probably be a button_to. Is there a more appropriate example we can use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, @ghiculescu ! I've added an expanded example of button_to that fails, and a short explanation on why link_to with data-turbo-method should be used instead in this commit: 1e568cd

@julianfssen julianfssen force-pushed the update-api-link-to-button-to-turbo branch from a4f89c0 to 1e568cd Compare March 3, 2023 12:13
@julianfssen
Copy link
Contributor Author

@ghiculescu , thank you for taking the time to review this PR! I've addressed your comments and looking forward to hearing back again when you're free. Thanks again!

@p8
Copy link
Member

p8 commented Mar 3, 2023

Hi @julianfssen 👋 .
Could you wrap the lines at 100 characters?

@julianfssen
Copy link
Contributor Author

Hi @julianfssen 👋 . Could you wrap the lines at 100 characters?

Thank you for feedback, @p8 !

I've added a commit (666c29a) to wrap the docs at 100 characters, which I believe starts from the comment character #? Do correct me if I'm wrong on this.

My bad as I wasn't aware of this guideline previously. I appreciate the feedback!

@julianfssen
Copy link
Contributor Author

PS: I've committed several fixup commits for easier referencing in comments, but will be squashing them once (and if) this PR is approved. Thank you everybody!

# button_to "Delete profile", @profile, method: :delete
#
# Since forms cannot appear inside another form,
# +button_to+ will not work here as it returns a form.
Copy link
Member

Choose a reason for hiding this comment

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

Separate PR, but we could probably have button_to warn or raise if it's called from inside a form_with.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I like that idea a lot

Copy link
Member

@ghiculescu ghiculescu Mar 3, 2023

Choose a reason for hiding this comment

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

I will make a PR
Edit: #47575

ghiculescu added a commit to ghiculescu/rails that referenced this pull request Mar 4, 2023
This is prohibited, per the HTML spec: https://developer.mozilla.org/en-US/docs/Learn/Forms/How_to_structure_a_web_form#the_form_element

This PR introduces `Rails.application.config.action_view.nested_form_behaviour`. This tries to alert the developer of nested forms. It works with `form_with`, `form_tag`, and `button_to`.

The config accepts `:raise`, `:log`, or `nil`. In 7.1, the default will be `:raise` in development and test. It will be `nil` in production, so no new errors will be raised.

ref: rails#47505 (comment) for the original idea
@julianfssen julianfssen force-pushed the update-api-link-to-button-to-turbo branch from 68e547c to 0ce255e Compare March 4, 2023 10:01
@julianfssen
Copy link
Contributor Author

Thank you for the feedback, @zzak ! I've addressed your comments and also squashed the commits for this PR. Appreciate the feedback again!

@brunoprietog
Copy link
Contributor

It sounds good to add it to the documentation, but this is really bad accessibility practice. Links should only be used for get actions. Anything else should be done with a button, for which there is the button_to helper with which you can achieve the same result by changing the method and action. If you need to execute actions inside forms, you can change them with a button containing the formmethod and formaction attributes without using links for it.

Considering this, I don't know if it is good to keep expanding the use of this by showing it in Rails documentation and guides.

What do you think? I know that this is not well known by many people, that is why I am not in favor of continuing to encourage its use.

@julianfssen
Copy link
Contributor Author

julianfssen commented Mar 7, 2023

It sounds good to add it to the documentation, but this is really bad accessibility practice. Links should only be used for get actions. Anything else should be done with a button, for which there is the button_to helper with which you can achieve the same result by changing the method and action. If you need to execute actions inside forms, you can change them with a button containing the formmethod and formaction attributes without using links for it.

Considering this, I don't know if it is good to keep expanding the use of this by showing it in Rails documentation and guides.

What do you think? I know that this is not well known by many people, that is why I am not in favor of continuing to encourage its use.

Thanks for the constructive feedback, @brunoprietog ! I agree this is bad for accessibility which is also why I added a note about it in the PR.

That said, using links to perform non-GET actions has been a thing ever since ujs (which is referenced in the API docs as well), so I don't think it it's new knowledge for a lot of Rails users.

The reason why I created this PR was because I was caught out by data-method and data-confirm not working when I upgraded my Rails app. I checked the Rails docs for link_to but could only find the deprecation note. I then discovered the solution through several Github issues and the Turbo handbook, which made me think it's a good idea to mention in the docs.

Of course, the right thing to do as you mentioned (which I agree) is to use a button or restructure the link to not use data-turbo-method.

However, I think it'd be more convenient for most developers to change data-method to data-turbo-method instead of rewriting their forms and links. I feel this is one of the rare moments where it's more practical for us developers to do it the "wrong" (?) way as data-turbo-method is extremely convenient for non-GET actions.

The PR is also not adding new behavior or suggesting the usage of a private API; data-turbo-method is exposed for this purpose and is mentioned in the Turbo handbook, which makes me feel it's fine to mention it in the Rails docs as well.

Let me know what you think of this! Thank you again for the feedback!

@julianfssen julianfssen requested review from zzak and ghiculescu and removed request for zzak March 13, 2023 15:02
@julianfssen
Copy link
Contributor Author

@ghiculescu @zzak thank you for the feedback! My bad for re-bumping this PR. May I know if the changes made above are good now? Thank you!

Comment on lines 198 to 199
# Using <tt>data-turbo-method</tt> is not ideal for accessibility
# but it is useful in scenarios like the above.
Copy link
Member

Choose a reason for hiding this comment

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

How does it violate a11y specifically? Is this something we can improve upon or are there workarounds?

It feels unnecessary to leave this comment here otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, @zzak ! I initially added this excerpt in as it was mentioned in the Turbo handbook. But, I agree it's unnecessary in the Rails docs.

@@ -239,6 +267,10 @@ def link_to(name = nil, options = nil, html_options = nil, &block)
# The form submits a POST request by default. You can specify a different
# HTTP verb via the +:method+ option within +html_options+.
#
# If the HTML button generated from +button_to+ does not work with your layout, you can
# consider using the +link_to+ method with the +data-turbo-method+
# attribute as described in the +link_to+ documentation.
Copy link
Member

Choose a reason for hiding this comment

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

The link_to documentation does not describe data-turbo-method yet or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This links back to the documentation I added for data-turbo-method in the link_to docs. We did the same previously for the :method argument so I thought of doing the same for data-turbo-method. However, I'm happy to remove it if we feel it's not necessary as well!

@julianfssen julianfssen force-pushed the update-api-link-to-button-to-turbo branch from 6238290 to 7361fcd Compare April 4, 2023 10:44
@julianfssen
Copy link
Contributor Author

Thank you for the re-review, @zzak ! I appreciate your help. I've updated the PR with your feedback, let me know what you think!

@julianfssen julianfssen requested a review from zzak April 4, 2023 10:45
@julianfssen julianfssen force-pushed the update-api-link-to-button-to-turbo branch from 7361fcd to 8ef5cec Compare July 2, 2023 13:31
@julianfssen
Copy link
Contributor Author

@zzak @ghiculescu , I'm bumping this PR again to decide if we should mention data-turbo-method and data-turbo-confirm in the link_to docs.

I appreciate your previous feedback on this. Do you mind giving it another review to see if it's worth an addition to the docs?

I apologize for the delay and I understand if we feel we don't need to add this in the docs. Thank you!

Comment on lines 183 to 191
# A common use case is to insert links to perform update or delete operations
# (i.e. non-<tt>GET</tt> requests) in a form.
#
# For example, you might use +button_to+ in an edit form to delete a record:
#
# button_to "Delete profile", @profile, method: :delete
#
# Since forms cannot appear inside another form,
# +button_to+ will not work here as it returns a form.
Copy link
Member

Choose a reason for hiding this comment

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

Is still find this whole section confusing. The bit about putting links inside forms being the most common usage doesn't sound right to me - is that actually the primary use case for this feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ghiculescu , you're right that it's likely not common to put links in forms.

I've updated the docs to be much more concise when documenting the data-turbo-method and data-turbo-confirm options. What do you think of this change?

# ==== Turbo
#
# Rails 7 ships with Turbo enabled by default. Turbo provides the following +:data+ options:
#
# * <tt>turbo_method: symbol of HTTP verb</tt> - Performs a Turbo link visit
# with the given HTTP verb. Forms are recommended when performing non-+GET+ requests.
# Only use <tt>data-turbo-method</tt> where a form is not possible.
#
# * <tt>turbo_confirm: "question?"</tt> - Adds a confirmation dialog to the link with the
# given value.
#
# {Consult the Turbo Handbook for more information on the options
# above.}[https://turbo.hotwired.dev/handbook/drive#performing-visits-with-a-different-method]
#
# ===== \Examples
#
# link_to "Delete profile", @profile, data: { turbo_method: :delete }
# # => <a href="/profiles/1" data-turbo-method="delete">Delete profile</a>
#
# link_to "Visit Other Site", "https://rubyonrails.org/", data: { turbo_confirm: "Are you sure?" }
# # => <a href="https://rubyonrails.org/" data-turbo-confirm="Are you sure?">Visit Other Site</a>
#

Rails 7 ships with Turbo enabled by default. Instead of using
`data-method` and `data-confirm`, Turbo now uses `data-turbo-method` and
`data-turbo-confirm` to perform a link visit with the specified HTTP
method and show confirmation dialog respective.

The deprecated legacy options are documented but the new options are
not.

This commit documents the `data-turbo-method` and `data-turbo-confirm`
for the `link_to` method.

The `button_to` documentation has also been updated to reference the
new `link_to` options.
@ghiculescu ghiculescu added the ready PRs ready to merge label Aug 16, 2023
@rafaelfranca rafaelfranca merged commit ae20b0d into rails:main Aug 21, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actionview ready PRs ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants