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

form_helper documentation using article instead of post [ci skip] #51376

Merged

Conversation

notapatch
Copy link
Contributor

@notapatch notapatch commented Mar 21, 2024

Motivation / Background

This Pull Request has been created because it will make the form_with documentation clearer.

Detail

Documentation previously uses post which is confusing as the methods/verb is also post.

Before
<form action="/posts" method="post">

After
<form action="/articles" method="post">

Method

Worked through a new Rails app with scaffold of article and checking the output of the HTML. Other HTML changes make it more authentic:

  • /> => at the end of the input
  • value attribute before name attribute

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes 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 Mar 21, 2024
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva left a comment

Choose a reason for hiding this comment

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

I agree "article" will read better than "post", but we should go ahead and change it in all the examples of this form helper file at least, there's more instances. Can you take a look at those?

actionview/lib/action_view/helpers/form_helper.rb Outdated Show resolved Hide resolved
@notapatch notapatch force-pushed the pr-form-with-update-documentation branch 2 times, most recently from fea9275 to 55bd9f1 Compare April 2, 2024 09:49
@notapatch
Copy link
Contributor Author

Hi @carlosantoniodasilva

The only references to post in the form_helper file are now related to methods.

Everything else is now article.

Otherwise

  • Added Fix you requested previously (assuming I understood properly!)
  • Removed a grammatical change.
  • Rebased just now.

Let me know if I can be of any assistance!

Copy link
Member

@carlosantoniodasilva carlosantoniodasilva left a comment

Choose a reason for hiding this comment

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

Nice work! I found just a handful more instances that could use some updating, other than that I'll ask you to squash your commits down to 1, and we're good to merge. Thanks!

actionview/lib/action_view/helpers/form_helper.rb Outdated Show resolved Hide resolved
actionview/lib/action_view/helpers/form_helper.rb Outdated Show resolved Hide resolved
actionview/lib/action_view/helpers/form_helper.rb Outdated Show resolved Hide resolved
actionview/lib/action_view/helpers/form_helper.rb Outdated Show resolved Hide resolved
Documentation previously uses post which is confusing as the
methods/verb is also post.

Before
<form action="/posts" method="post">

After
<form action="/articles" method="post">

Method
Worked through a new Rails app with scaffold of article
and checking the output of the HTML. Other HTML changes
make it more authentic:
/> => at the end of the input
@notapatch notapatch force-pushed the pr-form-with-update-documentation branch from 7e40c80 to 4a44b51 Compare April 10, 2024 14:24
@notapatch
Copy link
Contributor Author

Drat! I clearly didn't search for capital p Post!

Thanks for catching that Carlos. Fixed!

Work squashed.

@carlosantoniodasilva carlosantoniodasilva merged commit 6e551d2 into rails:main Apr 10, 2024
3 checks passed
@carlosantoniodasilva
Copy link
Member

Awesome, thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants