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

Include form_with in form_helpers rails guide #33523

Merged
merged 9 commits into from Aug 23, 2018

Conversation

Projects
None yet
4 participants
@Schwad
Contributor

Schwad commented Aug 3, 2018

Summary

This follows on from a discussion with @dhh and @kaspth regarding new form_with functionality in Rails 5.1 and updating our guides to reflect that. #25197

This commit appends new guide documentation based on the core docs and release notes

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Aug 3, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

rails-bot commented Aug 3, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@Schwad

This comment has been minimized.

Show comment
Hide comment
@Schwad

Schwad Aug 3, 2018

Contributor

@dhh I would appreciate it if you and @kaspth would take a look at this.

Besides any cursory tweaks ('include this, do not include that') I am in two minds on how we present form_with in the guide.

Part of me thinks that iteration one would be simply getting the documentation around form_with in the guides in its own section, as shown here. As suggested I simply used https://api.rubyonrails.org/classes/ActionView/Helpers/FormHelper.html#method-i-form_with and the release notes https://guides.rubyonrails.org/5_1_release_notes.html to inform the guide documentation.

If there is intent to move to form_with becoming the default Rails 'convention'; there should be discussion about a second iteration. That iteration could rewrite much of this guide to show form_with as the default interface and have cursory mentions of form_tag and form_for at the end. I am not saying we have to, but it's a discussion worth having.

Thanks much!

Contributor

Schwad commented Aug 3, 2018

@dhh I would appreciate it if you and @kaspth would take a look at this.

Besides any cursory tweaks ('include this, do not include that') I am in two minds on how we present form_with in the guide.

Part of me thinks that iteration one would be simply getting the documentation around form_with in the guides in its own section, as shown here. As suggested I simply used https://api.rubyonrails.org/classes/ActionView/Helpers/FormHelper.html#method-i-form_with and the release notes https://guides.rubyonrails.org/5_1_release_notes.html to inform the guide documentation.

If there is intent to move to form_with becoming the default Rails 'convention'; there should be discussion about a second iteration. That iteration could rewrite much of this guide to show form_with as the default interface and have cursory mentions of form_tag and form_for at the end. I am not saying we have to, but it's a discussion worth having.

Thanks much!

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Aug 10, 2018

Member

I think we should take the opportunity to completely revamp this guide. It should start out with the most common, conventional approach that most people are actually going to use. That's form_with. To start with this long exposition on form_tag, which most people and most apps should never use, doesn't make much sense to me.

We shouldn't even talk about form_for at all, except maybe as a footnote saying that this was an old style that's now soft deprecated.

Member

dhh commented Aug 10, 2018

I think we should take the opportunity to completely revamp this guide. It should start out with the most common, conventional approach that most people are actually going to use. That's form_with. To start with this long exposition on form_tag, which most people and most apps should never use, doesn't make much sense to me.

We shouldn't even talk about form_for at all, except maybe as a footnote saying that this was an old style that's now soft deprecated.

@Schwad

This comment has been minimized.

Show comment
Hide comment
@Schwad

Schwad Aug 10, 2018

Contributor
Contributor

Schwad commented Aug 10, 2018

@Schwad

This comment has been minimized.

Show comment
Hide comment
@Schwad

Schwad Aug 16, 2018

Contributor

The guide is revamped in this version. When we agree a final version I will be sure to squash. Deployed a copy of the latest revisions to Heroku for you to look at as well: http://revamped-form-helper-guide.herokuapp.com/

  • There are no more references to form_for or form_tag except for the footnote that points to older documentation.

  • All functionality that is now utilized with form_with has been updated.

  • There is a note about the remote: true default and local: true option.

  • There is a note about the ability for users to include non-attribute inputs on form builder now.

  • Notes that class and id are no longer required in HTML hash.

Look forward to your feedback, thank you! :)

Contributor

Schwad commented Aug 16, 2018

The guide is revamped in this version. When we agree a final version I will be sure to squash. Deployed a copy of the latest revisions to Heroku for you to look at as well: http://revamped-form-helper-guide.herokuapp.com/

  • There are no more references to form_for or form_tag except for the footnote that points to older documentation.

  • All functionality that is now utilized with form_with has been updated.

  • There is a note about the remote: true default and local: true option.

  • There is a note about the ability for users to include non-attribute inputs on form builder now.

  • Notes that class and id are no longer required in HTML hash.

Look forward to your feedback, thank you! :)

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Aug 21, 2018

Member

I think this is an improvement, but I'd like to continue after this going even further. The first example and introduction should be as this is most commonly used. Focus on the usage with models, since that's what everyone is going to do from the get-go. The example using get and /search is the exotic case, not the default case. Anyway happy to see this merged as an intermediary step.

Member

dhh commented Aug 21, 2018

I think this is an improvement, but I'd like to continue after this going even further. The first example and introduction should be as this is most commonly used. Focus on the usage with models, since that's what everyone is going to do from the get-go. The example using get and /search is the exotic case, not the default case. Anyway happy to see this merged as an intermediary step.

@Schwad

This comment has been minimized.

Show comment
Hide comment
@Schwad

Schwad Aug 22, 2018

Contributor
Contributor

Schwad commented Aug 22, 2018

@dhh dhh merged commit cdee520 into rails:master Aug 23, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Aug 23, 2018

Member

👍

Member

dhh commented Aug 23, 2018

👍

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Aug 27, 2018

Follow up #33523 [ci skip]
This commit is the next work after #33523.

Also, this commit removes mention about hidden `utf8` input. Since
form helpers don't generate this input by default since #32125.

Note that I also had created PR #31972 with improvements to
"Action View Form Helpers" guide, but I'll rebase it after merging the
current PR.

kamipo added a commit that referenced this pull request Aug 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment