Add a modal form option to the confirm and field entry form action link types

+++ b/config/schema/flag.schema.yml
@@ -123,6 +123,9 @@ flag.link_type.plugin.confirm:
+    modal:
+      type: boolean
+      label: 'Use modal dialog'

This should be a new link type plugin, not an extra setting.

Hmm good point...

I'm reluctant to add 2 more plugins... there would be a lot of code duplication.

Glancing at your patch, it looks like maybe this could be added to both the confirm and field entry plugins as a trait? There would be a 'modal' setting, but it would be on the plugin rather than on the flag.

Do any of the other maintainers and regular contributors have any thoughts on this?

Do any of the other maintainers and regular contributors have any thoughts on this?

I think a trait makes sense in this case.

So far, so good on this patch, but it needs tests.

+++ b/src/Plugin/ActionLink/ModalFormTrait.php
@@ -0,0 +1,48 @@
+      $render['#attributes']['data-dialog-type'] = 'modal';

I wonder whether one should be able to specfify the dialog type: modal, dialog or outside in thingy

So should #2834881: Extract out common interface in FieldEntry and ConfirmForm action link plugins be committed first?

Postponed until the above issue is in.

This looks good to me too.

Sorry to punt this back at this point... I've not been following this for a while.

1. +++ b/config/schema/flag.schema.yml
   @@ -137,6 +140,9 @@ flag.link_type.plugin.field_entry:
   +    modal:
   +      type: string
   +      label: 'Use modal dialog'
   

   This makes it sound like it should be a boolean....

2. +++ b/src/Plugin/ActionLink/FormEntryTypeBase.php
   @@ -62,6 +66,18 @@ abstract class FormEntryTypeBase extends ActionLinkTypeBase implements FormEntry
   +    $form['display']['settings']['link_options_' . $plugin_id]['modal'] = [
   +      '#type' => 'radios',
   +      '#title' => $this->t('Use a modal/dialog'),
   +      '#options' => [
   +        '0' => $this->t('None'),
   +        'dialog' => $this->t('Dialog'),
   +        'modal' => $this->t('Modal'),
   +      ],
   

   ... but it's in fact a range of options.

   Only *one* of which is called 'modal', so calling the overall thing modal is odd.

   Also, what does 'None' mean here? Doesn't it mean the site goes to a separate page load?

Yup, that makes more sense. 'form_behaviour' as the property name seems reasonable too.

By way of review.

1) Manual testing/UX - forms contain all the relevant text/information and button, and it functions and gives a good UX, which fits is well with all the has gone before.

My only comment in parsing is that the modal looks a bit old school, but that is the nature of modals - you either love or hate and we are putting that decision in the hands of the site owner. So I am happy ( Perhaps it would look better if the boxes of the model were rounded...but that is very much ore issue. )

2) PHP code look clean, sticks to all the coding standards, no surprises.

3) Testing in comprehensive, seems to completely cover the new functionality cleanly

So +1 from me

What follow is minor nits, found while running the new test through phpcs

 133 | WARNING | [ ] Do not concatenate strings to translatable
     |         |     strings, they should be part of the t() argument
     |         |     and you should use placeholders

I think I saw somebody in irc last week arguing that there should be less translate function calls in testing because unless you are testing specific language functions the default language in testing will always be english.

Anyway if testbot comes back green I would be happy to see this RTBC'ed

Looks good! Also works in views.

Thanks everyone! Really appreciate this patch, I hope a lot of people will find it useful.