Problem/Motivation

See #3281151: Remove extra margin on 'add paragraph' button on claro, the class is added to the button element, if modules like paragraphs are doing something arguably weird, button might not actually be a button.

It would IMHO be better if \Drupal\Core\Field\WidgetBase::formMultipleElements() would already set that button class instead of claro adding it itself, then other themes don't have to duplicate that.

Possible problems:

* Is it an API change to move a class like that? Nothing will change for any widget that does not replace the button, but for widgets that do, it will change back to a large button.

Steps to reproduce

Proposed resolution

Remaining tasks

* Create the patch to move that class from claro_preprocess_field_multiple_value_form() to the widget

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#21 core-adding-button-type-3292488.patch1.5 KBprimsi
#15 3292488-15.patch1.19 KB_utsavsharma
#15 interdiff_3-15.txt763 bytes_utsavsharma
#7 3292488-afterpatch.jpg6.41 KBgaurav-mathur
#7 3292488-beforepatch.jpg4.94 KBgaurav-mathur
#3 Screenshot_moveClassFromClaroThemeToWidget_CLARO_after.png11.51 KBromina_ferrario
#3 Screenshot_moveClassFromClaroThemeToWidget_CLARO_before.png11.1 KBromina_ferrario
#3 Screenshot_moveClassFromClaroThemeToWidget_SEVEN_after.png11.03 KBromina_ferrario
#3 Screenshot_moveClassFromClaroThemeToWidget_SEVEN_before.png10.71 KBromina_ferrario
#3 core_3292488_3_claro_moveClassFromClaroThemeToWidget.patch1.23 KBromina_ferrario

Issue fork drupal-3292488

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

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue summary: View changes
romina_ferrario’s picture

romina_ferrario commented
Status: Active » Needs review
StatusFileSize
new core_3292488_3_claro_moveClassFromClaroThemeToWidget.patch1.23 KB
new Screenshot_moveClassFromClaroThemeToWidget_SEVEN_before.png10.71 KB
new Screenshot_moveClassFromClaroThemeToWidget_SEVEN_after.png11.03 KB
new Screenshot_moveClassFromClaroThemeToWidget_CLARO_before.png11.1 KB
new Screenshot_moveClassFromClaroThemeToWidget_CLARO_after.png11.51 KB

move class button-small from claro_preprocess_field_multiple_value_form() to the widget

claro seven
before moving class screenshot of claro before screenshot of seven before
after moving class screenshot of claro after screenshot of seven after
rymcveigh’s picture

rymcveigh
he/him/his
commented

Just noting that the patch supplied in #3 will not apply when I attempt to apply it to Drupal version 9.4.5 via composer.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gaurav-mathur’s picture

gaurav-mathur commented
Assigned: Unassigned » gaurav-mathur
gaurav-mathur’s picture

gaurav-mathur commented
Assigned: gaurav-mathur » Unassigned
StatusFileSize
new 3292488-beforepatch.jpg4.94 KB
new 3292488-afterpatch.jpg6.41 KB

I apply patch #3 and it is work fine in claro.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

This looks like a good change and not breaking anything.

Also verified still applies to 10.1.x also.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: core_3292488_3_claro_moveClassFromClaroThemeToWidget.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Moving back as appears to be random failure for 9.5

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: core_3292488_3_claro_moveClassFromClaroThemeToWidget.patch, failed testing. View results

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Reviewed & tested by the community

Random test fail again I think.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Field/WidgetBase.php
@@ -262,7 +262,7 @@ protected function formMultipleElements(FieldItemListInterface $items, array &$f
+          '#attributes' => ['class' => ['field-add-more-submit', 'button--small']],

I think we could use the #button_type render array key instead. With that this would be just a render array change which is acceptable in a minor.

_utsavsharma’s picture

_utsavsharma commented
StatusFileSize
new interdiff_3-15.txt763 bytes
new 3292488-15.patch1.19 KB

Tried to address the pointer in #14.

Prashant.c made their first commit to this issue’s fork.

Prashant.c opened merge request !5815

prashant.c’s picture

prashant.c
he/him
Primary language English
Location Dharamshala
commented
Status: Needs work » Needs review
Issue tags: +ClaroContributionDay2023

Added '#button_type' => 'small' , kept the attributes 

'#attributes' => ['class' => ['field-add-more-submit']],

Raised a MR, changes needs to be reviewed.

Thanks!

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Seems the change at line 281 is out of scope. And issue summary mentions screenshots of a multi value widget which should be added to the user interface section of the summary

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

There is no change in core *if* you use Claro, we just move the class. Other themes that currently don't override it like that might change though. I suppose we could compare the backend with another theme, but not even sure which one?

primsi’s picture

primsi commented
StatusFileSize
new core-adding-button-type-3292488.patch1.5 KB

Patch for composer projects.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue summary: View changes
Status: Needs work » Needs review

Removing seven as I think that's no longer a concern. Gin extends from claro, so it inherits this, so nothing should change there either.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Hiding old patches.

Tested by adding an unlimited text field to a random content type. The "Add another item" button remained unchanged by the MR so no regression.

nod_ made their first commit to this issue’s fork.

nod_ closed merge request !5815

  • nod_ committed abcb2029 on 11.x 
    Issue #3292488 by berdir, prashant.c, romina_ferrario, _utsavsharma,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed abcb202 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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