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

Fix representation #4703

Merged
merged 2 commits into from
Jan 3, 2015
Merged

Fix representation #4703

merged 2 commits into from
Jan 3, 2015

Conversation

ifdattic
Copy link
Contributor

It might be just me, but with unless it sounds like if you want to use your forms in multiple places you should define it in your controllers.

Q A
Doc fix? yes
New docs? no
Applies to 2.3
Fixed tickets

It might be just me, but with `unless` it sounds like **if you want to use your forms in multiple places you should define it in your controllers**.


| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3
| Fixed tickets |
@xabbuh
Copy link
Member

xabbuh commented Dec 29, 2014

I think it's clear the way it is currently? What do others feel about it (especially other non-native speakers)?

@timglabisch
Copy link
Contributor

i think @xabbuh is right, we dont need to change this.

@javiereguiluz
Copy link
Member

In my opinion @ifdattic's version is easier to understand.

@wouterj
Copy link
Member

wouterj commented Dec 29, 2014

I agree with @ifdattic too. @weaverryan can you please decide and merge (or not)?

@xabbuh
Copy link
Member

xabbuh commented Dec 29, 2014

Let's change it then if the majority feels that it is more clear this way. 👍

@@ -13,7 +13,7 @@ Building Forms
Define your forms as PHP classes.

The Form component allows you to build forms right inside your controller
code. Honestly, unless you need to reuse the form somewhere else, that's
code. Honestly, if you don't need to reuse the form somewhere else, that's
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 the original is more clear, but I see your point. How about:

And if you don't need to reuse the form in multiple places, that's totally fine.

Copy link
Member

Choose a reason for hiding this comment

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

Then it becomes a bit unclear where "that" is refering too. What about:

The Form component allows you to build forms right inside your controller code. This is
perfectly fine if you don't need to reuse the form somewhere else.

Copy link
Member

Choose a reason for hiding this comment

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

I love it +1 from me

Copy link
Member

Choose a reason for hiding this comment

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

Nice suggestion! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 should I change it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please

@weaverryan
Copy link
Member

Awesome - thanks for the last update :)

@weaverryan weaverryan merged commit 6a55617 into symfony:2.3 Jan 3, 2015
weaverryan added a commit that referenced this pull request Jan 3, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

Fix representation

It might be just me, but with `unless` it sounds like **if you want to use your forms in multiple places you should define it in your controllers**.

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3
| Fixed tickets |

Commits
-------

6a55617 Update forms.rst
3d62349 Fix representation
@ifdattic ifdattic deleted the patch-3 branch January 4, 2015 08:08
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.

6 participants