Skip to content

[Cookbook][Dynamic Form Modification] Add AJAX sample #3411

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

Merged
merged 4 commits into from
Mar 18, 2014
Merged

[Cookbook][Dynamic Form Modification] Add AJAX sample #3411

merged 4 commits into from
Mar 18, 2014

Conversation

bicpi
Copy link
Contributor

@bicpi bicpi commented Jan 3, 2014

Q A
Doc fix? no
New docs? yes
Applies to 2.3+
Fixed tickets #2464 ("AJAX experience")

Do we need a PHP version of the create.html.twig which would mainly contain redundant JavaScript?

@wouterj
Copy link
Member

wouterj commented Jan 3, 2014

Do we need a PHP version of the create.html.twig which would mainly contain redundant JavaScript?

yes

->add('sport', 'entity', array(...))
->add('sport', 'entity', array(
'class' => 'AcmeDemoBundle:Sport',
'empty_value' => '',
Copy link
Member

Choose a reason for hiding this comment

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

please line out the => (same below)

@bicpi
Copy link
Contributor Author

bicpi commented Jan 4, 2014

Thanks for the review, issues are fixed now. Remaining question is if it's ok to use FOSJsRoutingBundle.

@weaverryan
Copy link
Member

@bicpi I vote yes to FOSJsRoutingBundle. I would like to see more instances where we basically say "this isn't core, but it's the standard tool people use to solve X". Obviously, we need to be careful to promote bundles that are agreed-upon. Fortunately, the FOS* bundles have kind of already gone through that process and are company-agnostic. So, I think it's an easy win to include it. Good proposal.


use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Template;
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 the @Route (very easy to understand, and short), but I'm not as excited about the @Template here. But, I may be splitting hairs. What do others think?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I'm against using one of those. But if we use one of them, we can use them both imo.

@stof
Copy link
Member

stof commented Jan 9, 2014

@weaverryan For FOSJsRoutingBundle, I think it is fine to recommend it: #796

@weaverryan
Copy link
Member

@bicpi I think this is ready to merge if you find some time for this last round of tweaks. I'm trying to get things perfect so that I can just hit the big merge button without making un-PR'ed commits of my own :).

Thanks!

@bicpi
Copy link
Contributor Author

bicpi commented Jan 23, 2014

I've updated this PR to the approach of submitting the whole form via AJAX. This means no JSON and no FOSJsRoutingBundle. I've also removed the ParamConverter, Route and Template annotations. Everything is reduced to the bare minimum now :-)

Please let me now if I can improve the naming schema for the rst.inc file or if we need a subfolder then for this cookbook or if if it doesn't make sense at all to create a partial for the (now greatly reduced) JS code - no problem to update this again.

Also, do we need a more detailed note at the end, maybe mentioning that the validation for AJAX form submission needs to fail?

@bicpi
Copy link
Contributor Author

bicpi commented Mar 16, 2014

@weaverryan Found another old but ready-to-merge PR, any further suggestions for this?

@weaverryan
Copy link
Member

Thanks for the poke - this is too good of an improvement to leave unmerged ;). Thans a lot for this!

weaverryan added a commit that referenced this pull request Mar 18, 2014
…bicpi)

This PR was merged into the 2.3 branch.

Discussion
----------

[Cookbook][Dynamic Form Modification] Add AJAX sample

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes
| Applies to    | 2.3+
| Fixed tickets | #2464 ("AJAX experience")

Do we need a PHP version of the `create.html.twig` which would mainly contain redundant JavaScript?

Commits
-------

a75ad9c Shorten AJAX example
ee33dcd Remove some empty lines from code samples
f47a7c3 Updates & Fixes after public review
2533f29 [Cookbook][Dynamic Form Modification] Add AJAX sample
@weaverryan weaverryan merged commit a75ad9c into symfony:2.3 Mar 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants