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

Guides 2.1: Add documentation for FormHelper #221

Merged
merged 14 commits into from
Nov 14, 2023

Conversation

swilgosz
Copy link
Member

@swilgosz swilgosz commented Nov 5, 2023

Resolves: #201

Copy link
Member

@jodosha jodosha left a comment

Choose a reason for hiding this comment

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

@swilgosz Thanks for this huge effort. 🙏

Please:

  1. Remove unneeded sections
  2. Accept inline comments, where it makes sense. Newline suggestions after H3 titles are for Markdown rendering reasons.
  3. Change ruby to erb in the examples

content/v2.1/helpers/form_helper.md Outdated Show resolved Hide resolved
content/v2.1/helpers/form_helper.md Outdated Show resolved Hide resolved
**Basic Example**

```ruby
<%= form_for("book", "/books", class: "form-horizontal") do |f| %>
Copy link
Member

Choose a reason for hiding this comment

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

@swilgosz It's time to use routing helpers 😉 .

content/v2.1/helpers/form_helper.md Outdated Show resolved Hide resolved

As you can see, all the fields had been prefixed with the form's name, which is a string `book`, passed as a first argument.

To show CSRF meta tags, you need to have [sessions enabled](/v2.0/actions/sessions). Without that, you won't have the `csrf_token` hidden input field available.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To show CSRF meta tags, you need to have [sessions enabled](/v2.0/actions/sessions). Without that, you won't have the `csrf_token` hidden input field available.

@swilgosz This is out of context.
You wrote a section that explains CSRF protection, maybe let's move it there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree. Because we have sessions disabled by default, but our examples assume it's enabled, I think it makes sense to mention the chapter when it's explained how to enable sessions. I've just removed the CSRF explanation, but kept the link

content/v2.1/helpers/form_helper.md Outdated Show resolved Hide resolved
content/v2.1/helpers/form_helper.md Outdated Show resolved Hide resolved
content/v2.1/helpers/form_helper.md Outdated Show resolved Hide resolved
content/v2.1/helpers/form_helper.md Show resolved Hide resolved
content/v2.1/helpers/form_helper.md Outdated Show resolved Hide resolved
@swilgosz swilgosz force-pushed the docs/guides-2.1-form-helpers branch from ba08159 to 0eed627 Compare November 7, 2023 00:50
@swilgosz swilgosz requested a review from jodosha November 8, 2023 09:22
@swilgosz
Copy link
Member Author

swilgosz commented Nov 8, 2023

@jodosha ready for another round of review, but erb snippets does not seem to be interpreted correctly.

Copy link
Member

@jodosha jodosha left a comment

Choose a reason for hiding this comment

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

@swilgosz Thank you very much for working on my previous suggestions. 🙏

We do not have a syntax highlighting for ERB snippets.
@swilgosz
Copy link
Member Author

Merging as the code had been approved.

@swilgosz swilgosz merged commit 3c72e55 into guides-2.1 Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants