Closed (fixed)
Project:
Paragraphs
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
18 May 2022 at 14:27 UTC
Updated:
1 Sep 2022 at 21:19 UTC
Jump to comment: Most recent, Most recent file


Comments
Comment #2
miro_dietikerThis is more a bugfix.
Also i don't like the horizontal indentation too much of the buttons. It is harder to track to which paragraph they belong with indentation.
Is this a must from claro designs? Is there no vertical alignment guide in form items?
Comment #3
miro_dietikerComment #4
berdirHm. web/core/themes/claro/templates/form/field-multiple-value-form.html.twig adds a form-actions wrapper around the add button, which causes a lot of extra margin.
For other add more buttons, that's not really a problem. They have the .field-add-more-submit class on the button that then reduces margin again. Lets try to add that our add more button? That's in \Drupal\paragraphs\Plugin\Field\FieldWidget\ParagraphsWidget::buildModalAddForm. Other add buttons have that, which is why it's not a problem for them.
The margin on the left side seems to come from having button--small on the add wrapper div. That is added by claro_preprocess_field_multiple_value_form to $variables ['button'], but button in this case is a div. That likely requires another patch for claro in core to only add that class if that element is actually submit type. Or maybe core should move that definition to core so that it's there in the default definition, making the concept of a small button not claro specific but something that other themes may also support and modules can rely on.
Comment #5
ckrinaIt took me a while to figure out this, so I'll add some screenshots to make it easier for whoever comes next.
Agreed with @Berdir: it looks like the main issue on a spacing perspective comes from adding
button--smalltoparagraphs-add-wrapper. That class is prepared to add padding & margin to the button itself, not wrappers. Removing this class would also have another good consequence: it'd remove the left spacing, which would left align the button with the top table.The form actions is also an extra 1rem top/bottom margin. So it'd be great to remove it too.
That would be great indeed and a very natural need for any UI relying on a design system.
I'm concerned about removing the assumption from code that the button itself needs to be small after a multiple value form. I have 0 idea of Paragraphs code (so sorry if this suggestion doesn't make any sense), but wouldn't it be better to not have a div behave as a button?
Comment #6
berdirThanks for the feedback.
> I'm concerned about removing the assumption from code that the button itself needs to be small after a multiple value form. I have 0 idea of Paragraphs code (so sorry if this suggestion doesn't make any sense), but wouldn't it be better to not have a div behave as a button?
That is a valid point, the situation is messy and arguably unexpected, the add modal form was basically bolted on top of the existing UI in a less-than-proper way. There might be ways we can rework that, but that also has a risk of breaking existing customizations and alters of the paragraph widget.
The reason it is a div is that it contains both the visible button as well as a hidden element that is shown as a dialog/modal when you click on the button. We could possibly put that somewhere else. But even then, we have other options that display multiple buttons, or a dropbutton in that space, we need a wrapper.
And you could argue that button is just strangely named. It's set in template_preprocess_field_multiple_value_form, which moves the add_more render key out to that button variable, to be able to display it separately. Lots of assumptions there, and directly tied to the render array structure that exists by default, but that field widgets can customize.
To be clear, I'm not suggesting to remove this entirely, just move it, from this preprocess function to \Drupal\Core\Field\WidgetBase::formMultipleElements(), where that element is originally defined. That would mean that other widgets that customize that add button would not get the small-button behavior, which might or might not be desired.
The problem with claro doing stuff in preprocess is that's impossible for modules to override that, themes are always last for preprocess logic, only a sub-theme could undo that element. Our only option would be to not use the same template but define our own for paragraphs, then we wouldn't get any of the claro-specific behaviors (and would possibly need to redo some of them).
> The form actions is also an extra 1rem top/bottom margin. So it'd be great to remove it too.
If you compare it with an add more button of a regular field, It looks like the approach in claro is to not remove the margin on the form-actions wrapper, but remove the inner markup of the button in this case:
.form-item--multiple .field-add-more-submit {
margin-top: 0;
margin-bottom: 0;
}
which kind of results in the same. (paragraphs module) just forgot to add this class in this case. IMHO the approach in claro is a bit strange, but we can work with that part to make it consistent with other add more buttons. Although I'm not sure why claro uses form-actions for element, only do undo its main effect again. system/stable uses a clearfix class on this element.
Comment #7
ckrinaGood points :)
I didn't explicitly said it, but I'm not against improving Claro's implementations at all. It can have a lot of implications, so I'd say involving @bnjmnm @lauriii would be a good move (both as Claro and as Front-end framework managers too).
Comment #8
berdirCreated the core issue #3292488: Move button--small class for add more field widget button to WidgetBase
Comment #9
hexabinaerComment #10
romina_ferrario commentedadded 'field-add-more-submit' to the add more button in the function buildModalAddForm
Comment #11
berdirThe patch contains unrelated changes.
Comment #12
romina_ferrario commentedOh, sorry, something must have gone wrong. Now it should be correct.
Comment #13
mathilde_dumond commentedComment #14
berdirCommitted.