Add paragraph before button

Cool stuff, hopefully others can review as well. A first quick review without testing:

It will be postponed until this #2946441: Add setting to hide the duplicate button (hopefully done today) and then need a feature setting check if the function is enabled and force the „...“ button to appear as it could be missing completely if no other thing is enabled and conditionally attach the library.

And tests :-)

Yesterday I was wondering why we are adding the button client side... Does it make sense?

We have more and more form elements that clutter everything and slowing down the form API.

The next iteration in the Paragraphs UI will move certain actions into the direction of the client.

I just committed the feature checkboxes in the UI. We can thus disable / enable new features like the duplicate or edit-all buttons.

This is a new feature that could be enabled by default, but needs an opt-out.

So let's add a new feature checkbox, check if it is enabled, and then make sure the ... button is always present.

The tests should disable all the other features and make sure that the button appears if the "add before" is the only action.
And yeah, sure the effective functionality of adding at the right position.

Hopefully we can get this in soon! :-)

Nice work.

I didn't go into implementation details, but I just want to mention here one thing that I have noticed when I started using "paragraph-type-add-modal-delta" hidden field and it could be relevant for this issue too.

When you click on "add before", hidden field will be filled with delta position and that value will stay for that field. When you click "Add paragraph" after that, because that's default adding of a new paragraph, delta will not be changed what will cause that a new paragraph will be added to the previous set position. And that's wrong.

For the improvements, that I'm working on it's not so relevant because I'm hiding "Add paragraph" and controlling delta always when a new paragraph is added by using new buttons. But in this case, you want to clear that state. Maybe the best way is to clear it with some behaviour attach or to change backend implementation to always set empty value (with the setting of empty value I had some side effects, but can't remember what was the problem).

About the add position problem:
Let's cover this in the modal dialog itself. If the dialog is cancelled, it can always reset the delta. (This is completely independent of the add-before JS code).

@miro_dietiker I agree with you and your proposal for the solution is good. Can you create a new issue for that?

@miro_dietiker How should we determine in JS whether the button should be added or not? A class on the actions? Or a attribute? This decision affects other client side actions in the future i guess.

I found it unintuitive to select the modal button so that the hidden inupt and therefore the add before buttons appear. How should we handle that?

> @miro_dietiker How should we determine in JS whether the button should be added or not? A class on the actions? Or a attribute?

Simple. You only add the library if the setting is enabled.

> I found it unintuitive to select the modal button so that the hidden inupt and therefore the add before buttons appear. How should we handle that?

Yeah, not perfect, but I don't see how other add types should work (We don't want to add an add button for every paragraph type for example), so for now, that's how it is. We can include a description in the UI that it only works if the modal is being used.

This also means that it should be off by default as it is not compatible with the default add mode.

Considering that there will be multiple variants of this, maybe there could be something like an API function that basically allows to open the add dialog with a certain position and makes sure to clean up after itself. We also need to make sure that it works with nested paragraph types. Maybe it already does, didn't really check the patch.

Issues that change the UI should always include screenshots, but I guess there's nothing too special about it, just another button inside the list .

We should still think about (how to maintain) button sequence.

@alex feel free to improbe the patch, i didn‘t check yet if it needs to improve for a commit, but the fact that it is opt in and isolated in its own library reduces my perfection stress a lot :-)
Keep in mind, beautification is optional. Configurability and minimal test coverage is a must. Thus my priorities are clear.

@mtodor @jenteralex Yes, I also think this is not related to this issue. This patch uses the weight currently set and writes the current value to the hidden field. The must be somewhere else.

@alexjenter

The other more meta question i have is, at the moment my implementation is pretty much just barely working and not very readable or sensible for that matter. I would like to refactor and make some smarter selector choices.

agreed. Specifically I'd like to improve those:

var $tableRow = $target.closest('tr');

var $submit = $widget.find('[type=submit]');

var $actions = $(context).find('.draggable .paragraphs-dropdown-actions').once().append(button);

Should we add classes to those elements to make them easier to select?

@berdir

Simple. You only add the library if the setting is enabled.

This doesn't work with nested paragraphs or multiple fields using paragraphs that use different settings. We need to know on which widget it has been enabled.

Yeah, not perfect, but I don't see how other add types should work (We don't want to add an add button for every paragraph type for example), so for now, that's how it is. We can include a description in the UI that it only works if the modal is being used.

Yeah, that's fine for me.

This also means that it should be off by default as it is not compatible with the default add mode.

I totally agree that this only works with the modal, but it is kind of sad that the user has to switch the button to get that feature. Any chance we can switch the default add mode?

Considering that there will be multiple variants of this, maybe there could be something like an API function that basically allows to open the add dialog with a certain position and makes sure to clean up after itself. We also need to make sure that it works with nested paragraph types. Maybe it already does, didn't really check the patch.

By cleaning up afterwards you mean resetting the hidden field to it's initial value?

Could you point us to the right direction for writing a test for this? Should we add them to ParagraphsExperimentalAddWidgetTest? What exactly should we test?

Here is a screenshot of the new button:

Updating remaining tasks

Testing:
In a widget with 3 items i would check if the new total „add before/above) button count is 3 after enabling the feature.

And then i would click the second, choose some type, and then you just assert the Paragraph was added at position 2.

More extensive tests about positioning are not needed here as we have it already from the hidden field.

I proposed a consistent button sequence in related issue.

That makes the "Add Paragraph before" the first item.
Let's consider that sequence in the patch already.

I'm promoting this as i want to have this in before we release.

The 'click' event didn't work for me for some reason, apparently 'mousedown' on its place does the trick.

Here a first version of the test, feedback welcome.

Not configurable yet, can work on this next.

The 'click' event didn't work for me for some reason, apparently 'mousedown' on its place does the trick.

I guess the parent element (the dropdown button) captures the click-event a little bit too greedy.
Wouldn't it be better to use "mouseup" instead of "mousedown"? With "mouseup" the user has a chance to cancel the click by simply moving the mouse away from the element.

@stBorchert Agree, mousedown is a bad thing, but it's what core does in all places (and it annoys me as well when trying to debug an item ba right clicking, while the ajax already fires)... We should start an initiative to get rid of all these items. :-)

Making it configurable (and disabled by default) was easier than expected :)

Some notes:

1) I feel that maybe the "Enable widget features" setting inside the widget configuration would benefit from a description for each feature. However this probably could be done in a follow-up for all existing features (if it doesn't exist already).

2) About the mousedown vs mouseup, a quick search in core returns 22 results for 'mousedown' and 4 for 'mouseup'. Should we go with the most used? :) Definitely no strong opinions here, and once I'm not a JS savvy, I defer to whatever you suggest.

Hm, queuing seems stuck, reuploading..

C'mon testbot

1. +++ b/js/paragraphs.add_before_button.js
   @@ -0,0 +1,51 @@
   +    $submit.click();
   +    // console.log({$tableRow, delta, $widget});
   

   debug left-over?

2. +++ b/js/paragraphs.add_before_button.js
   @@ -0,0 +1,51 @@
   +   * @prop {Drupal~behaviorAttach} attach
   +   *   Attaches paragraphsAddBeforeButton behaviors.
   +   */
   +
   +
   +  Drupal.behaviors.paragraphsAddBeforeButton = {
   

   strange empty lines?

3. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
   @@ -279,6 +279,7 @@ class ParagraphsWidget extends WidgetBase {
              'duplicate' => $this->t('Duplicate'),
              'collapse_edit_all' => $this->t('Collapse / Edit all'),
   +          'add_before' => $this->t('Add before'),
            ];
   

   this might need a bit more explanation/description?

   Miro asked what happens if you have nested paragraphs and enable it on *one* level only? Is the JS intelligent enough to handle that?
   (should be tested then if we don't do that yet..)

I investigated button sequence proposal here: #2948553: [META] Decide about Paragraph action sequence
So the button should be first, not last.

Also if i click this button on a node with many open top level Paragraphs, i get multiple overlays opened at once - plus multiple entity browsers open. There seems to be some odd .each or so. Not for nested closed Paragraphs though.

About naming: We always write capital "Paragraph", but i think we should just state "Add above" as short as all other actions.

And yes, if i enable this functionality on the host node field, it aggressively attaches itself to all nested child widgets. The configuration of the children is irrelevant. Instead we should only attach it where it's enabled.

Maybe we want to inject data attributes to a widget about the enabled features so we can offer a simple JS helper to check for enabled features?

This seems to work as well, and should be much less aggressive.

Also i see that the "Add above" button not only is added to the field instance items, but also the field instance table head label.

The result is the same - i get many overlays open when clicking.

+++ b/js/paragraphs.add_before_button.js
@@ -0,0 +1,53 @@
+      var $actions = $(context).find('.paragraphs-dropdown-actions');

This matches all "..." buttons of the widget, for field instance items and the field instance tableheader, and all nested ones as well. And then the party begins...

The action item is still the last in the "..." button for me instead of first.

Let's try again.

I've also tried to improve the test coverage with the nested scenarios, but couldn't finish it yet (stil WIP). But if you want to start testing it in parallel, in manual testing this works for me.

And now with a passing test for the nested paragraph scenario as well.

Note: I still think the UX here could perhaps be improved by adding some description texts to the features checkboxes... For example, if the user selects anything different than "Modal" for the add mode, the "Add before" feature doesn't make sense, and may confuse users. But this is probably better solved in a slightly more generic new issue.

Nice progress here!

I have tested this functionality a bit and also checked code.

1. +++ b/js/paragraphs.add_before_button.js
   @@ -0,0 +1,55 @@
   +    var delta = $tableRow.find('.delta-order select').val();
   

   You should not use the value from this select list. Problem is that delta is actually index (or position) and when you reorder paragraphs all values for selects are set in minus -> what makes adding of next paragraph to the first position. You can take a look how delta is calculated in "paragraphs_features" module for "add in between" feature.

2. +++ b/js/paragraphs.add_before_button.js
   @@ -0,0 +1,55 @@
   +      var button = '<button class="paragraphs-dropdown-action paragraphs-dropdown-action--add-before button js-form-submit form-submit">' + Drupal.t('Add above') + '</button>';
   

   I would suggest using input here since all other buttons are input "submit" elements and it would be good to keep it uniform. If you understand what I mean. That would be helpful for future styling, JS handling, etc.

3. +++ b/js/paragraphs.add_before_button.js
   @@ -0,0 +1,55 @@
   +      var $addButtons = $actions.find('.paragraphs-dropdown-action--add-before');
   

   I would suggest using once here, since I see "mousedown" registered multiple times.

4. +++ b/js/paragraphs.add_before_button.js
   @@ -0,0 +1,55 @@
   +      $addButtons.on('mousedown', clickHandler);
   

   For this one problem is in paragraphs.actions.js

           $toggle.on('focusout', function (e) {
             $this.removeClass('open');
           });
   

   That one goes before click and dropdown is hidden -> that's why click is not triggered, I guess. "mousedown" works because it's before "focusout". It would be nice to document that here at least or to change behaviour in paragraphs.actions.js. Maybe to add some debuance to handler function.

Yeah we should definitively have a separate issue about un‘messing that click handler..

As we had issues with duplicate handler triggers, using .once is a must.

Gonna continue with this, found a problem and still investigating, when:
- We add a paragraph above.
- We remove the new paragraph.
- Attempt to add a new paragraph above again.
The button in the latest patch refresh the page and opens the first paragraph, making the changes suggested above the button doesn't trigger anything at all now, checking.

I think this also fixes the problem in #45.

Need to work on tests now, if broken. :)

OK, updating the title, this seems to fix the tests and the styling of the button.

Should have updated this too, now adding tests for what I pointed in #45.
BTW, thank you so much @marcoscano for your patch, we are getting closer. :)

Please, we can pass now right? :)

T_T.

These tests are annoying...

(but necessary! :D )

1. +++ b/js/paragraphs.add_before_button.js
   @@ -0,0 +1,54 @@
   +    var $delta = $add_more_wrapper.closest('.clearfix')
   +      .find('.paragraph-type-add-modal-delta');
   

   Not too happy about using such a generic class here (.clearfix). But I see that this is added by field-multiple-value-form.html.twig, so there's not really a way to improve that. Except replacing that with our own template completely, which is obviously not a task for this issue.

   Also, we first above get the clearfix sibling to go down to the add dialog. Then here we go back up to the clearfix to go back down to the delta again. Would it make sense to save that element in a variable, so we don't have to find it a second time? And access both the hidden form field and the add wrapper from it?

2. +++ b/js/paragraphs.add_before_button.js
   @@ -0,0 +1,54 @@
   +    $delta.val(delta);
   

   We did not yet solve the cancel problem.

   To reproduce:

   1. Click on add above on any element.
   2. Cancel the dialog
   3. Click on the add button below
   4. Select a paragraph
   5. The paragraph is added above the item where you clicked add before.

   I think the easiest way to fix this is to adjust the code in paragraphs.modal.js, where we close/destroy the modal.

3. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
   @@ -279,6 +279,10 @@ class ParagraphsWidget extends WidgetBase {
              'duplicate' => $this->t('Duplicate'),
              'collapse_edit_all' => $this->t('Collapse / Edit all'),
   +          // The "Add before" feature will be completely injected clientside,
   +          // whenever this option is enabled in the widget configuration.
   +          // @see Drupal.behaviors.paragraphsAddBeforeButton
   +          'add_before' => $this->t('Add before'),
            ];
            break;
   

   It actually took me a minute to remember that I have to switch it to the modal, I think it would be really useful to have at least that explained to the user. I'd say we can extend the generic description text of the features to mention that for now.

I improved the description of the features and fixed 3.

The first two still need to be addressed. Still committed as it's our experimental widget and even opt-in only.

Added follow-ups.

Almost forgot this one.

Very nice!

Just one note regarding .clearfix

1. +++ b/js/paragraphs.add_before_button.js
   @@ -0,0 +1,54 @@
   +    var $add_more_wrapper = $button.closest('table')
   +      .siblings('.clearfix')
   +      .find('.paragraphs-add-dialog');
   ...
   +    var $delta = $add_more_wrapper.closest('.clearfix')
   +      .find('.paragraph-type-add-modal-delta');
   +    $delta.val(delta);
   

   In this case it's possible to just say var $add_more_wrapper = $button.closest('table').siblings().find('.paragraphs-add-dialog');
   That will search for paragraphs-add-dialog in all siblings. So that looks a bit cleaner and not generic selectors.

   And you can use same table for finding paragraph-type-add-modal-delta.

2. +++ b/js/paragraphs.add_before_button.js
   @@ -0,0 +1,54 @@
   +    var delta = $button.closest('tr').index();
   

   Ah, deltas. :'(
   This will not work nice with "add in between" feature, but it's fine for default paragraphs functionality.

   Maybe we could think about adding in paragraphs module some delta resolver (single function in paragraphs related namespace) that would work for everyone. It would resolve and return delta for paragraph row element passed in.

@mtodor can you transport that to the according follow-up please or open a new one? thx :-)

There is big potential to create some JS object that provides nice tools to interact with the widget.
I also proposed to inject other information into the DOM as data attributes and then we can offer JS getters.
This likely would simplify custom JS complexity and simplify future improvements like moving some (currently server) interaction (like deleting an item) into the client JS.

I have created an issue for the JS wrapper.