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

[RF-DOCS] Action View Form Helpers Guide [ci-skip] #51936

Merged
merged 53 commits into from
Jun 28, 2024

Conversation

bhumi1102
Copy link
Contributor

Motivation / Background

This Pull Request is for updating and improving the Action View Form Helpers Rails Guide documentation.

Details

In addition to updating the code examples and editing text for clarity and flow, here some more details on some of the changes:

  • Do we still need an HTML5 warning at the end of the 1st section? It's pretty standard these days.
    --> agree, removed.
  • Not sure the output of _form_with model: @article_ is 100% correct, might need to double check that.
    --> yes. updated this example and got the HTML output from my "guides playground" rails app.
  • When we mention record.persisted? in record identification, could be a good plug to link to the Active Model guide on that.
    --> Hm...there is no direction mention, other than this https://guides.rubyonrails.org/active_model_basics.html#conversion
  • Similar to STI, link to the guide / reference on it.
  • Time Zone and Country Select should likely be broken into separate sub-sections (I don't mind still mentioning country select)
  • Would the file upload example would be better with a CSV for local processing, rather than showing saving to local disk? (which is probably a very uncommon usage?)
  • The labeled_form_with example could likely be simplified with _**options_ being all that it takes, instead of explicitly showing all possible kwargs.
  • It may be better to show "complex forms" (section 10) right after the parameters (section 8), moving "forms to external resources" down... just because complex forms require exactly the fields_for incantation that was detailed further under the parameters section, so it seems a better continuation. (or potentially even reversed? complex forms then params? not sure)
  • Under "complex forms", adding fields on the flow could be slightly expanded, it feels very "go figure".
    --> Yeah, not sure what to do about this one. Was thinking about removing it as there is not built-in support to showcase.

Testing

  • Run guides:generate
  • Run guides:lint
  • Run rubocop on code samples

@rails-bot rails-bot bot added the docs label May 28, 2024
@p8 p8 added the rails foundation Rails Foundation PRs label May 28, 2024
Copy link
Contributor

@superchilled superchilled left a comment

Choose a reason for hiding this comment

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

Great job on this PR @bhumi1102 👍🏾

I made some suggestions in a few places. Hope it helps! 😃

guides/source/form_helpers.md Outdated Show resolved Hide resolved
guides/source/form_helpers.md Outdated Show resolved Hide resolved
guides/source/form_helpers.md Outdated Show resolved Hide resolved
guides/source/form_helpers.md Show resolved Hide resolved
guides/source/form_helpers.md Outdated Show resolved Hide resolved
guides/source/form_helpers.md Show resolved Hide resolved
guides/source/form_helpers.md Outdated Show resolved Hide resolved
bhumi1102 and others added 4 commits June 7, 2024 10:17
Co-authored-by: Karl Lingiah <karl@superchilled.co.uk>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
guides/source/form_helpers.md Outdated Show resolved Hide resolved
guides/source/form_helpers.md Outdated Show resolved Hide resolved
guides/source/form_helpers.md Outdated Show resolved Hide resolved
guides/source/form_helpers.md Outdated Show resolved Hide resolved
guides/source/form_helpers.md Outdated Show resolved Hide resolved
guides/source/form_helpers.md Outdated Show resolved Hide resolved
guides/source/form_helpers.md Outdated Show resolved Hide resolved
guides/source/form_helpers.md Outdated Show resolved Hide resolved
instead of `person[address][city]`. Thus we are able to determine which Address
records should be modified when processing the `params` hash.
rendered the `name` attribute of each city input as
`person[address][#{address.id}][city]` instead of `person[address][city]`. This
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 this whole section should use addresses instead of address for the form/parameters.
The association on person would probably be addresses as well and other examples above use addresses as well (this suggestion doesn't include all occurences as that was a bit diffuclt to do in Github 😞 )

Suggested change
`person[address][#{address.id}][city]` instead of `person[address][city]`. This
`person[addresses][#{address.id}][city]` instead of `person[addresses][city]`. This

Hmm, the next section implements this very similar example with nested attributes. Maybe this section should use a single address for a person instead?

Copy link
Contributor Author

@bhumi1102 bhumi1102 Jun 15, 2024

Choose a reason for hiding this comment

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

Hi @p8 - let me know if I misunderstood something but I double checked and it is indeed address as it is coming from within the loop @person.addresses.each do |address|:

  <% @person.addresses.each do |address| %>
    <%= person_form.fields_for address, index: address.id do |address_form| %>
      <%= address_form.text_field :city %>
    <% end %>
  <% end %>

The id specified using the index: option is what will then distinguish the addresses with person[address][23][city], I think that's the "feature" this section is trying to illustrate. Let me know if that make sense, or if I've misunderstood what you're getting at, happy to look into it more.

guides/source/form_helpers.md Outdated Show resolved Hide resolved
Comment on lines +1039 to 1040
Let's say you want to render a form with a set of fields for each of a person's
addresses. The [`fields_for`][] helper with its `:index` option can assist:
Copy link
Member

Choose a reason for hiding this comment

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

This fields_for currently isn't linked:
image

Suggested change
Let's say you want to render a form with a set of fields for each of a person's
addresses. The [`fields_for`][] helper with its `:index` option can assist:
Let's say you want to render a form with a set of fields for each of a person's
addresses. The [`fields_for`][] helper with its `:index` option can assist:
[`fields_for`]: https://api.rubyonrails.org/classes/ActionView/Helpers/FormBuilder.html#method-i-fields_for

@p8 p8 merged commit f0f624b into rails:main Jun 28, 2024
3 checks passed
p8 added a commit to p8/rails that referenced this pull request Jul 1, 2024
In addition to updating the code examples and editing text for clarity and flow, here some more details on some of the changes:

- [X] Do we still need an HTML5 warning at the end of the 1st section? It's pretty standard these days.
--> agree, removed.
- [X] Not sure the output of `_form_with model: @article`_ is 100% correct, might need to double check that.
--> yes. updated this example and got the HTML output from my "guides playground" rails app.
- [X] When we mention `record.persisted?` in record identification, could be a good plug to link to the Active Model guide on that.
--> Hm...there is no direction mention, other than this https://guides.rubyonrails.org/active_model_basics.html#conversion
- [X] Similar to STI, link to the guide / reference on it.
- [X] Time Zone and Country Select should likely be broken into separate sub-sections (I don't mind still mentioning country select)
- [X]  Would the file upload example would be better with a CSV for local processing, rather than showing saving to local disk? (which is probably a very uncommon usage?)
- [X] The _labeled_form_with_ example could likely be simplified with `_**options_` being all that it takes, instead of explicitly showing all possible kwargs.
- [X]  It may be better to show "complex forms" (section 10) right after the parameters (section 8), moving "forms to external resources" down... just because complex forms require exactly the fields_for incantation that was detailed further under the parameters section, so it seems a better continuation. (or potentially even reversed? complex forms then params? not sure)
- [X] Under "complex forms", adding fields on the flow could be slightly expanded, it feels very "go figure". 
--> Yeah, not sure what to do about this one. Was thinking about removing it as there is not built-in support to showcase.

---------

Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Amanda Perino <58528404+AmandaPerino@users.noreply.github.com>
Co-authored-by: Karl Lingiah <karl@superchilled.co.uk>
jianbo pushed a commit to jianbo/setter-with-association that referenced this pull request Jul 8, 2024
In addition to updating the code examples and editing text for clarity and flow, here some more details on some of the changes:

- [X] Do we still need an HTML5 warning at the end of the 1st section? It's pretty standard these days.
--> agree, removed.
- [X] Not sure the output of `_form_with model: @article`_ is 100% correct, might need to double check that.
--> yes. updated this example and got the HTML output from my "guides playground" rails app.
- [X] When we mention `record.persisted?` in record identification, could be a good plug to link to the Active Model guide on that.
--> Hm...there is no direction mention, other than this https://guides.rubyonrails.org/active_model_basics.html#conversion
- [X] Similar to STI, link to the guide / reference on it.
- [X] Time Zone and Country Select should likely be broken into separate sub-sections (I don't mind still mentioning country select)
- [X]  Would the file upload example would be better with a CSV for local processing, rather than showing saving to local disk? (which is probably a very uncommon usage?)
- [X] The _labeled_form_with_ example could likely be simplified with `_**options_` being all that it takes, instead of explicitly showing all possible kwargs.
- [X]  It may be better to show "complex forms" (section 10) right after the parameters (section 8), moving "forms to external resources" down... just because complex forms require exactly the fields_for incantation that was detailed further under the parameters section, so it seems a better continuation. (or potentially even reversed? complex forms then params? not sure)
- [X] Under "complex forms", adding fields on the flow could be slightly expanded, it feels very "go figure". 
--> Yeah, not sure what to do about this one. Was thinking about removing it as there is not built-in support to showcase.

---------

Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Amanda Perino <58528404+AmandaPerino@users.noreply.github.com>
Co-authored-by: Karl Lingiah <karl@superchilled.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs rails foundation Rails Foundation PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants