"Add another item" field button is displayed as a modal action

Looks good and works fine.

We still need tests, and it would be good to get a +1 from @lauriii

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

#42 works fine on my case: dialog with nested paragraph with multiple unlimited fields. Thank you very much.

I just applied the patch in #42 and the issue still occurs for me using Claro and Gin.

In my case, I had an actions structure within a fieldset in a form. My fix was to just remove the button from that inner actions. Then it corrected itself.

I'm not sure what button that is, but it's not the regular field widget one I think. The fix here is only about the default button in field widgets by changing the class to not use form-actions. form-actions is expected to be the primary action of a form I think and should be used for others.

Yeah that is coming from a form alter (mine) where I had action buttons within a fieldset. I just removed the actions part, it moved to the right place. But I saw having more than one actions referenced here:

https://git.drupalcode.org/project/examples/-/blob/4.0.x/modules/form_ap...

Hi, I've tested the patch on D10.0.9 + Gin theme and it works fine. Thank you.

Posted into #frontend channel but was previously tagged for tests which still need to happen.

I have been working on a test for this issue. I am uploading the test alone, and the test together with patch #65

phpcs fixes.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroModalDisplayTest.php
   @@ -0,0 +1,93 @@
   +/**
   + * Tests that buttons in modals are not in the footer.
   

   footer is probably not the right term for this, but I'm not exactly sure what is. The class for it is ui-dialog-buttonpan, so maybe "button pane"?

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroModalDisplayTest.php
   @@ -0,0 +1,93 @@
   +  /**
   +   * Tests that uploads in the 'media_library_widget' works as expected.
   +   */
   +  public function testWidgetUpload() {
   

   description and test method hasn't been updated from the copied example. maybe testModalAddAnother()?

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroModalDisplayTest.php
   @@ -0,0 +1,93 @@
   +    // A file needs to be added for the unlimited field to appear in the form.
   +    $this->addMediaFileToField('Add files', $this->container->get('file_system')->realpath($jpg_image->uri));
   +
   +    $selector = '.js-media-library-add-form-added-media';
   +    $this->assertJsCondition('jQuery("' . $selector . '").is(":focus")');
   

   maybe add a comment to the assert, that we wait for the upload to finish and refer where it's copied from?

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroModalDisplayTest.php
   @@ -0,0 +1,93 @@
   +
   +    // Assert that the 'add another item' button is not in the dialog footer.
   +    $assert_session->elementNotExists('css', '.ui-dialog-buttonset .field-add-more-submit');
   

   can you add a positive assert that it is where we expect it to be?

   Only having not asserts means that if for example the class changes, it will still pass but no longer test what we want it to. So add a very similar elementExists() and then that will fail and we have a reminder to update.

5. +++ b/core/themes/claro/css/components/action-link.css
   @@ -105,7 +105,8 @@
    
   -.form-actions .action-link {
   +.form-actions .action-link,
   +.multiple-value-form-actions .action-link {
      margin-inline: 0 var(--space-s);
    }
   

   I have no idea if we really need to change all places, but it is probably the safest option.

Thanks @berdir, I addressed all your comments.

phpcs fix

The last submitted patch, 71: 3151534-71-test-only.patch, failed testing. View results →

+++ b/core/themes/claro/templates/form/field-multiple-value-form.html.twig
@@ -42,7 +42,7 @@
+      <div class="multiple-value-form-actions">{{ button }}</div>

I think it would be nice to name this a bit more generically.. Maybe something like field-actions could work?

Updated the class as per #72, attached interdiff for same

Removing "Needs frontend framework manager review" tag because I don't think this actually needs a framework manager sign-off and I have reviewed this as a maintainer of Claro. Also removing "Needs tests" because there are tests in the patch, and test only patch in #67.

Solution seems simple enough 👍 Removing @nest from action-link CSS is a bit of scope creep but seems at least fine by me. Claro is internal so we could probably backport this to 10.1.x as a non-disruptive bug fix.

The patch in #74 does not apply. Can we have a reroll?

Simple reroll with a conflict in core/themes/claro/css/components/form.pcss.css.

The last submitted patch, 77: 3151534-77-reroll.patch, failed testing. View results →

Sorry for the noise.. I broke HEAD in other issue... 🤦‍♂️

Attached is patch 77 rolled for 10.1.x

Updated patches based on #77:
- 11.x: re-rolled for latest changes to 11.x
- 10.1.x: code cleanup

The last submitted patch, 81: 3151534-D10.1.x-based-on-77--81.patch, failed testing. View results →

Looks like another random fail.

Automatically closed - issue fixed for 2 weeks with no activity.