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

[Cookbook] Fix doc on Generic Form Type Extensions #5227

Closed

Conversation

lemoinem
Copy link
Contributor

@lemoinem lemoinem commented May 4, 2015

Q A
Doc fix? yes
New docs? no
Applies to >=2.3
Fixed tickets N/A

Follow up on #5154 (Sorry the first one didn't use the PR Template BTW) and symfony/symfony#14456.

I cleaned up the reference on DIC Tags, I think referring to the cookbook article prevents duplicating documentation (and taking the risk it will get out of date or inconsistent later on, as it did).

Then I removed the mention of Generic FTE at the top of the cookbook article. Since the feature doesn't exists anymore, I think it's better to leave it aside. I simply mentioned why it's impossible in a short last paragraph at the end of the CB entry.

I kept both commit unsquashed because they seemed to address different enough concerns. But I will squash them upon request if you think it's necessary.

@lemoinem
Copy link
Contributor Author

It's been more than a month, any news?

Generic Form Type Extensions
----------------------------

Although it is not possible to have a Form Type Extension applying to all Form
Copy link
Member

Choose a reason for hiding this comment

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

form type extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There, it's fixed.

@lemoinem lemoinem force-pushed the fix/no-generic-form-type-extension branch from 87a0d4a to f1f5292 Compare June 15, 2015 13:10
@lemoinem
Copy link
Contributor Author

I fixed the typo mentioned by @xabbuh. If you have any other feedback to provide, I'd be glad to hear it.

@xabbuh
Copy link
Member

xabbuh commented Jun 15, 2015

@lemoinem Can you please change that everywhere in the parts you modified (the same by the way also applies to "Form Type").

@lemoinem lemoinem force-pushed the fix/no-generic-form-type-extension branch from f1f5292 to 465ffdd Compare June 15, 2015 13:26
@lemoinem
Copy link
Contributor Author

@xabbuh Done! (:

@xabbuh
Copy link
Member

xabbuh commented Jun 23, 2015

@lemoinem This looks great now! Could you do another rebase please (sorry for that, but we had some activity on the repo the last days)?

Avoid duplicating documentation by redirecting to the cookbook entry (as many
other entries do).
@lemoinem lemoinem force-pushed the fix/no-generic-form-type-extension branch from 465ffdd to a309be8 Compare June 23, 2015 18:34
@lemoinem
Copy link
Contributor Author

@xabbuh Thanks for the feedback!

Here is the rebase. Enjoy!

@xabbuh
Copy link
Member

xabbuh commented Jun 23, 2015

👍

@javiereguiluz
Copy link
Member

👍 thanks for this @lemoinem Reducing doc duplication is a big plus for us!

adding a "help" text to every field type);
#. You want to add a **specific feature to a single type** (such
as adding a "download" feature to the "file" field type).

Copy link
Member

Choose a reason for hiding this comment

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

I like these 2 bullet points - they quickly answer the question "what can I do with this form type extension thing?". I think you removed them because it says "every field type", but that's effectively true (the button being the exception). I'd like to have these back, and let the last paragraph explain the edge case.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this was a great way to explain Foem Type Extensions.

I restored them but changed the example. I now use ""adding a "help" text to every "input text"-like type"". This seem to be closer to an actual use case but also more truthful regarding what can be done.

@lemoinem lemoinem force-pushed the fix/no-generic-form-type-extension branch from 87efc95 to 855301d Compare June 29, 2015 19:26
@lemoinem
Copy link
Contributor Author

@weaverryan : Thanks for the feedback. Here is a new tentative version with a more positive wording.

Let me know what you think about it. Once we all agree on a better version, I will squash the commits and rebase.

@lemoinem
Copy link
Contributor Author

@weaverryan @xabbuh
It's been a bit more than 2 months since the last modification regarding this PR. Any feedback? Is the last wording appropriate? If so, Should I squash the commits so it could all be merged?

@weaverryan
Copy link
Member

Thanks for the ping! I just left one small comment - but a 👍 from me! A merger can even add that small change of we get to it first :) (unfortunately I'm on my phone now!)

@lemoinem
Copy link
Contributor Author

@weaverryan
I added a mention regarding getExtendedType. A more detailed code block seemed redundant with the rest of the article (especially with the Defining a Form Type Extension section).

If everything is OK, I'll squash it all and rebase.

xabbuh added a commit that referenced this pull request Sep 23, 2015
…nem)

This PR was squashed before being merged into the 2.3 branch (closes #5227).

Discussion
----------

[Cookbook] Fix doc on Generic Form Type Extensions

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

Follow up on #5154 (Sorry the first one didn't use the PR Template BTW) and symfony/symfony#14456.

I cleaned up the reference on DIC Tags, I think referring to the cookbook article prevents duplicating documentation (and taking the risk it will get out of date or inconsistent later on, as it did).

Then I removed the mention of Generic FTE at the top of the cookbook article. Since the feature doesn't exists anymore, I think it's better to leave it aside. I simply mentioned why it's impossible in a short last paragraph at the end of the CB entry.

I kept both commit unsquashed because they seemed to address different enough concerns. But I will squash them upon request if you think it's necessary.

Commits
-------

a6bc497 [Cookbook] Fix doc on Generic Form Type Extensions
xabbuh added a commit that referenced this pull request Sep 23, 2015
@xabbuh
Copy link
Member

xabbuh commented Sep 23, 2015

Thank you @lemoinem. I have merged your changes (your commits have been squashed, that's why the pull request is shown as closed) and made some minor tweaks to them in c035fb0.

@xabbuh xabbuh closed this Sep 23, 2015
@lemoinem lemoinem deleted the fix/no-generic-form-type-extension branch September 23, 2015 21:16
@lemoinem
Copy link
Contributor Author

Thank you very much for the follow up!

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.

5 participants