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

Provide form_with as a new alternative to form_for/form_tag #25197

Closed
dhh opened this issue May 30, 2016 · 35 comments
Closed

Provide form_with as a new alternative to form_for/form_tag #25197

dhh opened this issue May 30, 2016 · 35 comments
Assignees

Comments

@dhh
Copy link
Member

@dhh dhh commented May 30, 2016

form_tag and form_for provide two very similiar interfaces to much of the same thing. We should unify that usage and subsequent calls to field tags. Additionally, there are several deficiencies with the current helpers that I'd like to solve at the same time, now that we will have a new form method to change defaults with:

  1. Don't set DOM id or classes on the form or the fields any more. It frequently created duplicate ids and its not used often enough to warrant being a default behavior. You can easily add the id or class if need be.
  2. Don't require HTML tags, like class and id, to be wrapped in a html: {} key. There aren't a high likelihood of conflict and it complicates the default cases.
  3. Allow form fields that do not correspond to model attributes. This makes it easier to mix and match a form with some fields that correspond to a model and others that will trigger other behaviors (like sending a welcome email).
  4. Make remote: true the default. Full-page changes after submissions are rough. When using Turbolinks, a normal redirect will generate a Turbolinks.visit() call, and otherwise there's SJR. (We could consider having config.action_view.forms_remote_by_default that you could set to false, for people going old school).

Examples:

# Passing model: @post will 1) set scope: :post, 2) set url: url_for(@post)
form_with(model: @post) do |form|
  form.text_field :title # Will reference @post.title as normal
  form.text_area :description, "Overwrite @post.description if present, if not, it will still work"

  form.submit
end

form_with(scope: :post, url: posts_path) do |form|
  form.text_field :title # post[title]
  form.text_area :description, "Overwrite @post.description or ignore if it's not present"

  form.submit
end

# Submits title=X&description=Y
form_with(url: different_path, class: 'something', id: 'specific') do |form|
  form.text_field :title, 'This has is the value of the title'

  form.text_area :description, class: 'No value has been supplied here'

  form.fields(:permission) do |fields|
    # on/off instead of positional parameters for setting values
    fields.check_box :admin, on: 'yes', off: 'no'
  end

  form.select :category, Post::CATEGORIES, blank: 'None'
  form.select :author_id, Person.all.collect { |p| [ p.name, p.id ] }, blank: 'Pick someone'

  form.submit
end

There's still a fair amount of design work to deal with, especially around the FormOptionsHelper. We should move away from positional parameters entirely and replace them with named ones. But we should also try to cut down on needless options and pick better defaults. And finally we should simply drop a lot of the overly-specialized select option methods.

Note: For 5.x, form_for and form_tag should just be soft deprecations, no warnings. Then we can deprecate with a warning, perhaps, in Rails 6.x.

@dhh dhh added this to the 5.1.0 milestone May 30, 2016
@vipulnsward
Copy link
Member

@vipulnsward vipulnsward commented May 30, 2016

I can give this a try.

@sgrif
Copy link
Contributor

@sgrif sgrif commented May 30, 2016

Ideally we could implement form_for and form_tag on top of this and move it to a gem, which would lower the maintenance cost if we choose not to deprecate it (It would be incredibly painful to deprecate those)

@dhh
Copy link
Member Author

@dhh dhh commented May 30, 2016

That'd work for me.

On May 30, 2016, at 20:15, Sean Griffin notifications@github.com wrote:

Ideally we could implement form_for and form_tag on top of this and move it to a gem, which would lower the maintenance cost if we choose not to deprecate it.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@kaspth
Copy link
Member

@kaspth kaspth commented Jun 28, 2016

@dhh how would you create a nested form in this way? Just pass an array to :url?

form_with(model: Comment.new, url: [ @post, Comment.new ]) do |form|
  # ...
end
@dhh
Copy link
Member Author

@dhh dhh commented Jun 28, 2016

No, I would stick with that as a parameter to model. So it's form_with(model: [ @post, Comment.new ]) do ...

@marekkirejczyk
Copy link
Contributor

@marekkirejczyk marekkirejczyk commented Jun 28, 2016

I just created a pull request with a prototype - far from ready solution yet.

My considerations so far:

  • Is the general approach right (new module, new builder, using tag helpers (ie text_field) in implementation, note: I assume FormWithBuilder will not be inheriting from FormBuilder in final implementation).
  • Do we plan to deprecate tag helpers (ie text_field) at some point?
  • Implementation of url only forms (ie form_with(url: different_path) ...) - seems to be far away from model or url+scope (ie form_with(scope: :post, url: posts_path)). I think about using separate builder based on pure tag helpers (ie text_field_tag instead of text_field) would be a good idea here (one abstract problem - building tags, to concrete implementations depending on parameters), but would make it more difficult to create custom builders.
  • Any thoughts on the syntax of following?

Allow form fields that do not correspond to model attributes

Your feedback will be appreciated :)

@kaspth @dhh @vipulnsward

@marekkirejczyk
Copy link
Contributor

@marekkirejczyk marekkirejczyk commented Jul 10, 2016

Just updated PR (view here). All @dhh examples from this PR are now covered.

Questions:

If id are not set by default, how do we handle for attribute in labels? i.e.:

<label for=?>Title</label>
<input type='text' name='post[title]' value='Catch 22' />

For select, collection_select, etc helpers will benefit from named parameters, i.e.

Before:

f.select :category, categories, { include_blank: "Select one" }, class: "nice"
f.collection_select(:post, :author_name, :authors, :id, :name, { include_blank: "Select one" }, class: "nice"

After:

f.select :category, categories, blank: "Select one" , class: "nice"
f.collection_select :post, :author_name, :authors, :id, :name, blank: "Select one", class: 'beauty'

Is there anything more clever we can do here?

Form fields now are allowed not correspond to model attributes. i.e.:

f.text_field :title
f.text_field :title, scope: 'custom'
f.text_field :title, scope: nil

Will generate:

<input type='text' name='post[title]' value='...' />")
<input type='text' name='custom[title]' value='...' />")
<input type='text' name='title' value='...' />")

Looks good?

On implementation side:

  • Builder now uses Tag::Base family instead of form tags helpers, so it will be easy to deprecate helpers.
  • Still todo: nested forms, get rid of old form_builder as a base, tests for all the fields methods and cases, docs

Your feedback will be appreciated @kaspth @dhh @vipulnsward.

@dhh
Copy link
Member Author

@dhh dhh commented Jul 11, 2016

  • Labels: Let's see what it looks like when the label code just requires an id to be set manually on the element.
  • select/collection_select: Not sure I understand the question? Looks like using named stuff is just nicer?
  • Form fields: Look good, but we should be using HTML5 tags, so close with > not />.
@marekkirejczyk
Copy link
Contributor

@marekkirejczyk marekkirejczyk commented Jul 11, 2016

@dhh

Labels

Labels: Let's see what it looks like when the label code just requires an id to be set manually on the element

I see 3 options:

  • Option 1: Manually add for and id:
f.label(:title, "The Title")      # or f.label("The Title", for: 'title')
f.text_field :title, id: 'title'

translates to:

<label for='title'>The Title</label>
<input type='text' name='post[title]' id='title'/>
  • Option 2: We can recommend to use nesting feature of labels:
f.label("The Title") do
  f.text_field :title
end

or shorter version:
f.label("The Title") { f.text_field :title }

or even shorter - label as an extra param of text_field:
f.text_field :title, label: "The Title"

which would translate to:

<label>The Title <input type='text' name='post[title]' /></label>
  • Option 3: Keep the old semantics and generate id i.e.
f.label :title, "The Title"
f.text_field :title

translates to:

<label for='post_title'>The Title</label>
<input type='text' name='post[title]' value='Catch 22' />

select helpers

select/collection_select: Not sure I understand the question? Looks like using named stuff is just nicer?

Is there anything else we would like to do (as we already touching *_select helpers) other then introduce named parameters and revisit parameter names?

@kaspth
Copy link
Member

@kaspth kaspth commented Jul 11, 2016

Whatever we decide with labels if would be nice to tie them to checkboxes if that's the field you're rendering. I think that's a good default for Rails to help out with. Which we could solve with the :label option.

@dhh I'm curious what you mean by this in your original comment:

form.text_area :description, "Overwrite @post.description if present, if not, it will still work"

Why should it only overwrite the description if present? And how will it work if the @post has no description?

Is there anything else we would like to do (as we already touching *_select helpers) other then introduce named parameters and revisit parameter names?

I think we have to drop down to actual code to be able to spot that.

@jonathanhefner
Copy link
Member

@jonathanhefner jonathanhefner commented Jul 12, 2016

Perhaps ids could still be generated for use with <label for="">, but it could be made obvious that they are generated. For example, choose random number when the builder is instantiated (in case there are multiple builders per page) and concat that number with an internal counter which is incremented per field.

Regardless of id generation, I like the f.text_field :title, label: "The Title" API. Using label: false could skip the label, and a default of label: true could produce whatever f.label :title would. This would give the built-in form builder most of the brevity that popular (but bloated) form builder gems have, which I think is their chief selling point.

@marekkirejczyk
Copy link
Contributor

@marekkirejczyk marekkirejczyk commented Jul 12, 2016

@jonathanhefner We could also add parameter to form_for tag, to generate (or not) labels by default for given form:

form_with model: @post, labels: true do
...
end
marekkirejczyk added a commit to marekkirejczyk/rails that referenced this issue Jul 13, 2016
@sevos
Copy link

@sevos sevos commented Mar 21, 2017

I will try to isolate the problem!

@kaspth
Copy link
Member

@kaspth kaspth commented Mar 21, 2017

@sevos So there is an issue here after all, thanks for reporting! Since form_with defaults to remote forms it's best we embed authenticity tokens in the forms, so people don't have to rely on rails-ujs.

Currently the workaround is to either bundle rails-ujs or set config.action_view.embed_authenticity_tokens_in_remote_forms = true in application.rb.

👆 @samandmoore yup, when that config's enabled it works.

@kaspth
Copy link
Member

@kaspth kaspth commented Mar 25, 2017

Also: long since closed through #26976 and follow up commits.

@krtschmr
Copy link

@krtschmr krtschmr commented Apr 19, 2017

@kaspth regarding the token issue: if remote is false and token is true, there still no tokens rendered

#28796

ticolucci added a commit to ticolucci/rails-girls-il that referenced this issue May 10, 2017
The Installation guides do not specify a rails version:
`gem install rails --no-document`

Therefore the new people will install rails 5.1 (as of today).
One of the view changes was the introduction of `form_with` instead of `form_for`:
rails/rails#25197

I updated both the 'replace the following code' sections as well as the edit code to
follow the new helper standards. (using the variable `form` instead of `f` \o/)
@PrimeTimeTran
Copy link

@PrimeTimeTran PrimeTimeTran commented Jan 1, 2018

Sorry I'm a little bit late to the party, but would you guys mind giving me a pointer?
https://stackoverflow.com/questions/48053022/rails-5-1-strong-parameters-deprecated

I've read the repo's code and I'm still confused. Is there no way to use from_with for a nested route? For example, users/:user_id/friendships/:id

Sorry for the newb question! I read through the comments thoroughly and didn't spot an example of that.

Maybe if I understand I can help you guys out with documentation in the future? lol. I've always wanted to contribute to Rails

@Schwad
Copy link
Contributor

@Schwad Schwad commented Jul 27, 2018

Not sure if this is mentioned elsewhere, but do we need to update actionview guides to reflect this addition? https://guides.rubyonrails.org/form_helpers.html

Lots of talk about form_tag and form_for but no form_with. If someone can verify I'm more than happy to take a crack at it. Thank you! :)

@dhh
Copy link
Member Author

@dhh dhh commented Jul 27, 2018

@kaspth
Copy link
Member

@kaspth kaspth commented Jul 28, 2018

@Schwad looks like I forgot to add it, please do! You should be able to copy a bunch of things from
https://api.rubyonrails.org/classes/ActionView/Helpers/FormHelper.html#method-i-form_with

@Schwad
Copy link
Contributor

@Schwad Schwad commented Aug 10, 2018

Okay, I've whipped together a PR. Would appreciate a peek from @dhh or @kaspth .

Essentially I did what you recommended @kaspth - I informed the documentation from what you put together there and formatted it for the guide. I think if form_with does become the default convention (it's all I use now) then a second PR should be considered to put it ahead of form_for and form_tag in the guides.... But for now this at least includes the existing documentation in the full guide.

#33523

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.