Problem/Motivation

For example, using starterkit_theme or demo_umami the button is too small as it picks up font-size: 0.85em;.

Steps to reproduce

  1. Create a site based on starterkit_theme
  2. Ensure that "Use the administration theme when editing or creating content" is not checked
  3. Create a node type with a media reference field that allows multiple values
  4. Edit a node of this type

Proposed resolution

Move the button outside of the div.description.

Remaining tasks

User interface changes

There is a minor visual difference in the appearance of "Add another item" buttons due to the change in font size from .85em to 1em.

Before:
A screenshot showing a media reference field with its "Add another item" button enclosed in the field description

After:
A screenshot showing a media reference field with its "Add another item" button outside the field description

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#7 after.png17.01 KBdcam
#7 before.png16.81 KBdcam
#6 after.jpeg23.08 KBdcam
#6 before.jpeg20.47 KBdcam

Issue fork drupal-3616675

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

adamps created an issue. See original summary.

adamps opened merge request !16701

adamps’s picture

adamps commented
Issue summary: View changes
Status: Active » Needs review

The test fail appears to be unrelated to this fix.

dcam’s picture

dcam commented
Status: Needs review » Needs work
Issue tags: +Needs change record, +Needs tests

I was able to use the steps to reproduce the issue. I was pretty sure that this is an in appropriate placement of the "Add another..." button, but I also added a few other field types and none of them included the button in the description. So the change looks appropriate to me.

Since this is a template change, it needs a change record so that themers can be warned of the change. It should include "before" and "after" examples of the markup. I'm setting the status to Needs Work for that. See an example at https://www.drupal.org/node/3523777.

I also think that this will need regression tests. So I'm tagging the issue for that.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

I am not convinced that many themes will be overriding this? I'm also not sure it needs tests, the only test I can think of is one that validates the specific HTML structure, but I don't think we can test the bug directly as described.

dcam’s picture

dcam commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record, -Needs tests
StatusFileSize
new before.jpeg20.47 KB
new after.jpeg23.08 KB

@longwave If you don't think that tests and a CR are necessary, then I'm not going to argue with you. But I will point out precedent from a similar issue involving descriptions, #3459838: Form element #field_prefix is shown before #description. In that issue we added tests that, yes, verified the HTML structure. And I wrote a change record. So my review comments were based on that.

Since there is a minor visual difference I've added screenshots to the issue summary.

dcam’s picture

dcam commented
Issue summary: View changes
StatusFileSize
new before.png16.81 KB
new after.png17.01 KB

Those were the wrong screenshots.

dcam’s picture

dcam commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

I think a small change record here would be great - just for themers. I agree with no tests.

From a semantic sense, moving the add another item outside of the description div is correct. Does this result in any change in claro or default admin - as these are the normal themes where this button will appear.

Once the default admin/claro question and the change record exists this can go back to RTBC.

dcam’s picture

dcam commented
Issue tags: -Needs change record

Does this result in any change in claro or default admin...?

Yes. default_admin overrides this template. Claro does not as far as I can tell. Neither does the Starterkit theme.

I wrote a change record, but will leave the status at Needs Work for the default_admin change.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

So should we be making the same change to the default admin theme - I think this change has semantic benefits too - the button is not part of the description. And does it result in changes if we move it... and what about claro?