Use bootstrap modals instead of javascript confirm #263

calvera · 2016-03-03T13:28:39Z

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 · 2016-03-03T15:13:24Z

Hi @calvera

Sounds like a good idea!

I'll check the PR ASAP ;)

Thanks,

bobvandevijver · 2016-03-03T18:42:11Z

Resources/templates/CommonAdmin/NestedListTemplate/NestedListBuilderTemplate.php.twig

+    </div>
+  </div>
+</div>
+{{ echo_endblock() }}


This looks like a duplication of the code in the ListBuilderTemplate. So, it would be better to create a separate template which is included here: also makes it easier to customize :)

Good point, I will deduplicate

sescandell · 2016-03-04T07:04:05Z

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)

sescandell · 2016-03-04T07:09:18Z

Resources/templates/CommonAdmin/object_actions.php.twig

@@ -35,17 +35,23 @@

    {% spaceless %}
        {{ echo_spaceless() }}
+        <div {% if builder.yamlKey is same as('list') or builder.yamlKey is same as('nested_list') %}


Are you sure this new "parent" doesn't break buttons display?
Given that object actions are wrapped by this code:

<div class="col-md-12 btn-group"> {{ block('object_actions') }} </div>

I'm wondering if:

<div class="col-md-12 btn-group"> <div> <a class="btn btn-default">button1</a> <a class="btn btn-default">button2</a> <a class="btn btn-default">button3</a> <div> </div>

Would be rendered the same as:

<div class="col-md-12 btn-group"> <a class="btn btn-default">button1</a> <a class="btn btn-default">button2</a> <a class="btn btn-default">button3</a> </div>

(Just a question, I don't remember how BS handle btn-group)

addition div is wrong, span would be better, but i will remove addional tag at all, which is the best

about dynamic forms:
I think, the correct behavior would be:

confirmation is needed -> show bs modal and POST a form with or without csrf protection if confirmed

no confirmation and csrf protection -> POST a dynamic form with csrf

no confirmation and no protection -> do not change, href link is followed

Do you agree?

calvera · 2016-03-04T08:02:34Z

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

calvera · 2016-03-05T00:21:59Z

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 · 2016-03-07T07:11:21Z

Resources/public/js/main.js

-        $(this.options.containerSelector).on('click', this.options.buttonSelector, this.clickHandler.bind(this));
+        $(this.options.containerSelector)
+            .find(this.options.buttonSelector)
+            .on('click',this.clickHandler.bind(this));


Could you please come back to the previous implementation here. The difference is small, but previous version permit to handle AJAX loaded content (while the find version doesn't AFAIK).

yes, I will do, thanks for noticing this

sescandell · 2016-03-07T07:18:49Z

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 · 2016-03-11T12:18:34Z

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

sescandell · 2016-03-13T18:35:37Z

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 · 2016-03-13T20:15:34Z

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 · 2016-03-13T20:37:09Z

@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 · 2016-03-14T07:44:08Z

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 · 2016-03-14T17:43:54Z

@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 · 2016-03-26T19:55:44Z

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

sescandell · 2016-03-28T15:42:56Z

Looks good to me.

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

Many thanks @calvera

sescandell · 2016-03-31T06:27:56Z

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

thanks

calvera · 2016-03-31T07:36:43Z

squashed and rebased

sescandell · 2016-03-31T08:01:14Z

Merged, many thanks @calvera 👍

bobvandevijver reviewed Mar 3, 2016
View reviewed changes

sescandell reviewed Mar 4, 2016
View reviewed changes

sescandell reviewed Mar 7, 2016
View reviewed changes

sescandell added the ready to review label Mar 10, 2016

sescandell self-assigned this Mar 10, 2016

sescandell added the awaiting feedback label Mar 10, 2016

sescandell added this to the v2.2 milestone Mar 29, 2016

use bootstrap modal for confirmation instead javascript confirm

c411f73

calvera force-pushed the modals branch from cb11ef2 to c411f73 Compare March 31, 2016 07:34

sescandell merged commit fab86a7 into symfony2admingenerator:master Mar 31, 2016

calvera deleted the modals branch March 31, 2016 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use bootstrap modals instead of javascript confirm #263

Use bootstrap modals instead of javascript confirm #263

calvera commented Mar 3, 2016

sescandell commented Mar 3, 2016

bobvandevijver Mar 3, 2016

calvera Mar 4, 2016

sescandell commented Mar 4, 2016

sescandell Mar 4, 2016

calvera Mar 4, 2016

sescandell Mar 4, 2016

calvera commented Mar 4, 2016

calvera commented Mar 5, 2016

sescandell Mar 7, 2016

calvera Mar 7, 2016

sescandell commented Mar 7, 2016

calvera commented Mar 11, 2016

sescandell commented Mar 13, 2016

ioleo commented Mar 13, 2016

calvera commented Mar 13, 2016

sescandell commented Mar 14, 2016

calvera commented Mar 14, 2016

calvera commented Mar 26, 2016

sescandell commented Mar 28, 2016

sescandell commented Mar 31, 2016

calvera commented Mar 31, 2016

sescandell commented Mar 31, 2016

Use bootstrap modals instead of javascript confirm #263

Use bootstrap modals instead of javascript confirm #263

Conversation

calvera commented Mar 3, 2016

sescandell commented Mar 3, 2016

bobvandevijver Mar 3, 2016

Choose a reason for hiding this comment

calvera Mar 4, 2016

Choose a reason for hiding this comment

sescandell commented Mar 4, 2016

sescandell Mar 4, 2016

Choose a reason for hiding this comment

calvera Mar 4, 2016

Choose a reason for hiding this comment

sescandell Mar 4, 2016

Choose a reason for hiding this comment

calvera commented Mar 4, 2016

calvera commented Mar 5, 2016

sescandell Mar 7, 2016

Choose a reason for hiding this comment

calvera Mar 7, 2016

Choose a reason for hiding this comment

sescandell commented Mar 7, 2016

calvera commented Mar 11, 2016

sescandell commented Mar 13, 2016

ioleo commented Mar 13, 2016

calvera commented Mar 13, 2016

sescandell commented Mar 14, 2016

calvera commented Mar 14, 2016

calvera commented Mar 26, 2016

sescandell commented Mar 28, 2016

sescandell commented Mar 31, 2016

calvera commented Mar 31, 2016

sescandell commented Mar 31, 2016