Use bootstrap modals instead of javascript confirm

[calvera]

I'm not sure if you are interested in...

this PR does not introduce BC break... nothing needs to be changed in the code...

please comment...

[sescandell]

Hi @calvera

Sounds like a good idea!

I'll check the PR ASAP ;)

Thanks,

[sescandell]

Hi,

I've seen some little things you need to change to make this working as expected.

csrf-token does not always exist. The action is not necessaraly protected. If you look at the previous code, we used a custom form only if the action is protected.

Previous behavior was:

• If action is protected: prevent default event, generate a dynamic form and submit it
• If action is not protected: let default behavior continue

If I'm not wrong, in your new implementation:

• If the action need a confirmation, you will always use a dynamic form: it might not be the necessary if the action is not protected
• If the action doesn't need a confirmation but is protected, it doesn't use the dynamic form (whereas it should)

[calvera]

thank you for your comments, i'll improve the PR

[calvera]

so, now all should work, object actions, batch actions and generic actions as well. I have tested it on demo https://github.com/symfony2admingenerator/symfony2-admingenerator-demo-edition

[sescandell]

Hi @calvera

I see you added the confirmModal property on Action objects, but I don't see Action class(es). Could you please double check that point (or remove the confirmModal option, that's your choice, I see what you mean by this parameter).

If you let the confirmModal option, please, update the documentation to recise there is a new option confirmModal in generators for actions, possible values and behavior depending on value.

Thank you,

[calvera]

some rework, for demo see https://github.com/calvera/symfony2-admingenerator-demo-edition/tree/modals

[sescandell]

Hi @calvera

Everything seems good... I'm just wondering if your proposition about modals with additional fields is the right way to recommend. I agree with you: it works, and is fairly simple. But, with that solution I think you miss something important which is Symfony Form bidning and all validation process. What if the field required a valid unique email but is not valid (because already in the database). With this solution, the user doesn't have an easy way to manage that situation.

I already pushed a solution to that following an issue if I'm not wrong, based on Ajax, real form and managing validation constraints (actually based on Get / Post requests and form submitted or not). Could you please so complete your documentation based on that?

[ioleo]

I agree with @sescandell. The idea is good, but this should be improved to handle the form via AJAX and display server-side validation messages.

[calvera]

@sescandell i'm not sure what you mean by 'I already pushed a solution to that following an issue...'. What you pushed and where? I have an example of full Symfony form in my symfony2-admingenerator-demo-edition/src/Admingenerator/DoctrineOrmDemoBundle/Resources/views/PostActions/index.html.twig

but this is not ideal... i'm not sure if we could somehow make all action twig templates to be ajax compatible...

[sescandell]

Hi @calvera

We discussed about that there is some times now here and following comment.

I took a look to your fork of the demo. In that code, you create a new modal with a form not binded to any Symfony2 Form (here. My proposition is to manage that more globally with dynamic forms (as discussed here a long time ago).

Actually, if I clearly understand your fork, this is really near to how you handle simpleedit action (but loaders and form handlers are maybe missing).

If you prefer, I can propose you to split this PR in 2 parts: first one is moving to bootstrap modals (what you already made and is pretty good IMO) ; then, secondly, AJAX and dynamic form loading in modals (with documentation and/or helpers).

[calvera]

@sescandell - the ajax/form part is now in cookbook only. I think someone could use the 'no-form' approach for simple task. it is his/her decision. let's make it clear in cookbook.

but I agree we could leave the full ajax/form for another PR.

thanks for comments and proposals...

p.s. after i read that discussion about ajaxification, i'll think about it.

[calvera]

Where are we now? Am I supposed to do something more? :)

[sescandell]

Looks good to me.

Ping @bobvandevijver and @loostro. If everybody is ok, I'll merge it :+1:

Many thanks @calvera

[sescandell]

@calvera could you please rebase the PR (otherwise, I'll do it :))

thanks

[calvera]

squashed and rebased

[sescandell]

Merged, many thanks @calvera :+1: