- Status changed to Needs review
almost 2 years ago 5:30am 30 January 2023 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
We still need tests, and it would be good to get a +1 from @lauriii
- Status changed to Needs work
almost 2 years ago 12:00am 31 January 2023 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.
- 🇮🇹Italy finex
#42 works fine on my case: dialog with nested paragraph with multiple unlimited fields. Thank you very much.
- 🇺🇸United States kevinquillen
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.
- 🇨🇭Switzerland berdir Switzerland
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.
- 🇺🇸United States kevinquillen
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...
- last update
over 1 year ago Custom Commands Failed - @darren-oh opened merge request.
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - @darren-oh opened merge request.
- last update
over 1 year ago 29,420 pass - last update
over 1 year ago 30,338 pass - last update
over 1 year ago 28,523 pass - @darren-oh opened merge request.
- last update
over 1 year ago 29,437 pass - @darren-oh opened merge request.
- Status changed to Needs review
over 1 year ago 3:52pm 11 June 2023 - last update
over 1 year ago 29,437 pass - last update
over 1 year ago 29,420 pass - 🇮🇹Italy finex
Hi, I've tested the patch on D10.0.9 + Gin theme and it works fine. Thank you.
35:54 32:01 Running35:26 32:01 Running35:22 32:01 Running35:17 32:01 Running- last update
over 1 year ago 29,448 pass - Status changed to Needs work
over 1 year ago 6:03pm 14 June 2023 - 🇺🇸United States smustgrave
Posted into #frontend channel but was previously tagged for tests which still need to happen.
- 🇨🇭Switzerland mathilde_dumond
I have been working on a test for this issue. I am uploading the test alone, and the test together with patch #65
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,446 pass, 2 fail - last update
over 1 year ago 29,447 pass - Status changed to Needs review
over 1 year ago 9:06am 21 July 2023 - Status changed to Needs work
over 1 year ago 9:40am 21 July 2023 - 🇨🇭Switzerland berdir Switzerland
-
+++ 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"?
-
+++ 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()?
-
+++ 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?
-
+++ 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.
-
+++ 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.
-
- Status changed to Needs review
over 1 year ago 11:56am 21 July 2023 - Open on Drupal.org →Environment: PHP 8.2 & MySQL 8last update
over 1 year ago Waiting for branch to pass - 🇨🇭Switzerland mathilde_dumond
Thanks @berdir, I addressed all your comments.
- last update
over 1 year ago 29,840 pass - last update
over 1 year ago 29,446 pass, 2 fail - last update
over 1 year ago 29,447 pass The last submitted patch, 71: 3151534-71-test-only.patch, failed testing. View results →
- 🇫🇮Finland lauriii Finland
+++ 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? - last update
over 1 year ago 29,912 pass - 🇮🇳India gauravvvv Delhi, India
Updated the class as per #72, attached interdiff for same
- Status changed to RTBC
over 1 year ago 9:37am 1 August 2023 - 🇫🇮Finland lauriii Finland
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. - last update
over 1 year ago 29,947 pass - last update
over 1 year ago 29,954 pass - last update
over 1 year ago 29,954 pass - last update
over 1 year ago 29,959 pass - last update
over 1 year ago 29,959 pass - last update
over 1 year ago Patch Failed to Apply - last update
over 1 year ago 30,334 pass, 4 fail - last update
over 1 year ago 29,959 pass - last update
over 1 year ago 29,960 pass - last update
over 1 year ago 29,972 pass - last update
over 1 year ago 30,050 pass - last update
over 1 year ago Patch Failed to Apply - last update
over 1 year ago Patch Failed to Apply - Status changed to Needs work
over 1 year ago 10:36am 24 August 2023 - 🇳🇿New Zealand quietone
The patch in #74 does not apply. Can we have a reroll?
- Status changed to RTBC
over 1 year ago 10:58am 24 August 2023 - last update
over 1 year ago 30,056 pass, 1 fail - 🇫🇮Finland lauriii Finland
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 →
- last update
over 1 year ago 30,059 pass - 🇫🇮Finland lauriii Finland
Sorry for the noise.. I broke HEAD in other issue... 🤦♂️
- last update
over 1 year ago 30,061 pass - last update
over 1 year ago 30,061 pass - last update
over 1 year ago 30,099 pass - last update
over 1 year ago 30,135 pass - last update
over 1 year ago 30,136 pass - last update
over 1 year ago 30,137 pass - last update
over 1 year ago 30,137 pass - last update
over 1 year ago 30,147 pass - last update
over 1 year ago 30,149 pass - last update
over 1 year ago Patch Failed to Apply - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,477 pass, 2 fail - last update
over 1 year ago 30,155 pass - 🇺🇸United States recrit
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 →
- last update
over 1 year ago 29,478 pass - last update
over 1 year ago 30,378 pass - last update
over 1 year ago 30,383 pass - last update
about 1 year ago 30,393 pass - last update
about 1 year ago 30,395 pass - last update
about 1 year ago 30,398 pass - last update
about 1 year ago 30,412 pass - Status changed to Fixed
about 1 year ago 8:46am 18 October 2023 - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago 29,673 pass Automatically closed - issue fixed for 2 weeks with no activity.