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.
* If and how it will affect seven

Steps to reproduce

Proposed resolution

Remaining tasks

* Create the patch to move that class from claro_preprocess_field_multiple_value_form() to the widget
* Create screenshots for seven for a regular multi-value widget

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH for Schlütersche Fachmedien GmbH commented
Issue summary: View changes
romina_ferrario’s picture

romina_ferrario CreditAttribution: romina_ferrario commented
Status: Active » Needs review
FileSize
core_3292488_3_claro_moveClassFromClaroThemeToWidget.patch1.23 KB
Screenshot_moveClassFromClaroThemeToWidget_SEVEN_before.png10.71 KB
Screenshot_moveClassFromClaroThemeToWidget_SEVEN_after.png11.03 KB
Screenshot_moveClassFromClaroThemeToWidget_CLARO_before.png11.1 KB
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
CreditAttribution: rymcveigh at Morris Animal Foundation 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 CreditAttribution: gaurav-mathur at Dotsquares Ltd. commented
Assigned: Unassigned » gaurav-mathur
gaurav-mathur’s picture

gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commented
Assigned: gaurav-mathur » Unassigned
FileSize
3292488-beforepatch.jpg4.94 KB
3292488-afterpatch.jpg6.41 KB

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

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo 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 CreditAttribution: smustgrave at Mobomo 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
CreditAttribution: Berdir at MD Systems GmbH for Schlütersche Fachmedien GmbH 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
CreditAttribution: lauriii at Acquia 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 CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commented
FileSize
interdiff_3-15.txt763 bytes
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
CreditAttribution: Prashant.c as a volunteer and at QED42 for QED42 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 CreditAttribution: smustgrave at Mobomo 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
CreditAttribution: Berdir at MD Systems GmbH for Schlütersche Fachmedien GmbH 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?