-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
Consistently render button_to
as <button>
#40747
Conversation
I now realize this is a re-imagining of #35143 (authored by @duleorlovic). My changeset doesn't include any of the configurations from that branch. Is re-enabling |
Yeah, we still want that configuration. Don't like the Let's add @duleorlovic as a co-author for good measure as well. |
I mean unless you can find a suitable reason to not use a configuration point for this. I haven't looked into this so I'm repeating from @rafaelfranca. |
e518b92
to
5f2cf30
Compare
9e216ef
to
6324fd5
Compare
...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_6_2.rb.tt
Show resolved
Hide resolved
...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_6_2.rb.tt
Show resolved
Hide resolved
b736b7b
to
ca7f4bd
Compare
@@ -50,6 +50,8 @@ class UrlHelperTest < ActiveSupport::TestCase | |||
include RenderERBUtils | |||
|
|||
setup :_prepare_context | |||
setup { ActionView::Helpers::UrlHelper.button_to_generates_button_tag = true } | |||
teardown { ActionView::Helpers::UrlHelper.button_to_generates_button_tag = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaspth since the default is false
, these lines were necessary to enable the new behavior.
Does this feel idiomatic and compatible with the new_framework_defaults pattern to you?
Currently, this commit only includes a single test to cover button_to
without a block with button_to_generates_button_tag = false
. Is that sufficient? Would it be better to double the collection of def test_button_to_*
tests to ensure we have coverage for both variations of the configuration flag?
ca7f4bd
to
d7b2dc7
Compare
...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_6_2.rb.tt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Can you squash the last commit?
Prior to this commit, the [ActionView::Helpers::UrlHelper#button_to][button_to] helper rendered `<input type="submit">` elements when passed its contents as a String argument, and rendered `<button type="submit">` elements when passed its contents as a block. This difference is subtle, and might lead to surprises. Additionally, a `<form>` element's submitter can encode a `name`/`value` pairing, which will be submitted as part of the request. When `button_to` renders an `<input type="submit">` element, the "button" content is rendered as a `[value]` attribute, which prevents any meaningful data from being encoded. Since it's a single `<button>` or `<input type="submit">` within a `<form>`, missing out on that opportunity to encode information might not be a show stopper, but ensuring that a `<button>` element is rendered _without_ a default `[value]` attribute enables applications to encode additional information that can be accessed JavaScript as `element.value`, instead of a workaround like `element.getAttribute("data-value")`. Support rendering `input` elements with button_to --- To support the original behavior of `button_to` rendering `<input type="submit">` elements when invoked _without_ a block, expose the `app.config.button_to_generates_button_tag` configuration flag. By default, it's set to `true` and ensures that all `button_to` calls render `<button>` elements. To revert to the original behavior, set it to `false`. [button_to]: https://api.rubyonrails.org/v6.0/classes/ActionView/Helpers/UrlHelper.html#method-i-button_to Co-authored-by: Dusan Orlovic <duleorlovic@gmail.com>
53a747b
to
9af9458
Compare
@rafaelfranca I've squashed the commits. Thank you for the review! |
In rails#40747 we added `ActionView::Helpers::UrlHelper.button_to_generates_button_tag`. - `config.action_view.button_to_generates_button_tag` should configure `ActionView::Helpers::UrlHelper.button_to_generates_button_tag` - Added test cases - Fixed new_framework_defaults_6_2.rb.tt (See rails#40747 (comment), rails#40747 (comment), cc @kaspth) - Added the option to the configuring guide - Documented - Added to "6.2 defaults section - Added to "Baseline defaults" section (see ff88113)
Rails 7 made `#button_to` consistent [1], so that now it always render a `<button>` HTML tag. Before that, its output was an `<input>` tag in some circumstances. Our tests were relying on the output to be an `<input>` tag, but we can be generic and catch both the old and new behaviors. See [2] for the line of code in solidus were the tag is generated. [1] - rails/rails#40747 [2] - https://github.com/solidusio/solidus/blob/350ed34e54fe27ff4e7ef8ea7478dbd5372796b9/backend/app/helpers/spree/admin/orders_helper.rb#L12
ref: rails#40747 The global config is great for new projects, but for large existing projects, auditing every `button_to` call could be overwhelming enough to not bother. This PR allows you to set if a button should always be returned per-invocation. This way you can gradually opt in to the config, or you can use it only in parts of your codebase. ```ruby button_to("Save", "http://www.example.com") # renders <input> button_to("Save", "http://www.example.com", button_tag: true) # renders <button> button_to("Save", "http://www.example.com", button_tag: false) # renders <input> button_to("Save", "http://www.example.com") # renders <button> button_to("Save", "http://www.example.com", button_tag: true) # renders <button> button_to("Save", "http://www.example.com", button_tag: false) # renders <input> ```
Would love feedback on #49142 |
Consistently render
button_to
as<button>
Prior to this commit, the
ActionView::Helpers::UrlHelper#button_to helper rendered
<input type="submit">
elements when passed its contents as a Stringargument, and rendered
<button type="submit">
elements when passed itscontents as a block.
This difference is subtle, and might lead to surprises.
Additionally, a
<form>
element's submitter can encode aname
/value
pairing, which will be submitted as part of the request. When
button_to
renders an<input type="submit">
element, the "button"content is rendered as a
[value]
attribute, which prevents anymeaningful data from being encoded.
Since it's a single
<button>
or<input type="submit">
within a<form>
, missing out on that opportunity to encode information mightnot be a show stopper, but ensuring that a
<button>
element isrendered without a default
[value]
attribute enables applications toencode additional information that can be accessed JavaScript as
element.value
, instead of a workaround likeelement.getAttribute("data-value")
.Support rendering
input
elements with button_toTo support the original behavior of
button_to
rendering<input type="submit">
elements when invoked without a block, expose theapp.config.button_to_generates_button_tag
configuration flag.By default, it's set to
true
and ensures that allbutton_to
callsrender
<button>
elements. To revert to the original behavior, set itto
false
.Co-authored-by: Dusan Orlovic duleorlovic@gmail.com