Problem/Motivation

Follow-up of #3109571: Hide "Add above" when switching to the behavior tab

Currently, the Add Above feature works only with the Modal Add mode. If the Add Mode is set to other than Modal, the form submits empty values and refreshes making the user lose all data.

Proposed resolution

Either just allowing Add Mode when Moda is the Add Mode set or make the Add Above feature work with the other Add modes.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#21 interdiff-3133439-18-21.txt2.32 KBjohnchque
#21 3133439-disable-add-above-modal-21.patch17.61 KBjohnchque
#21 3133439-disable-add-above-modal-21-NO-TESTS.patch7.13 KBjohnchque
#18 interdiff-3133439-16-18.txt2.72 KBjohnchque
#18 3133439-disable-add-above-modal-18.patch15.14 KBjohnchque
#18 3133439-disable-add-above-modal-18-NO-TESTS.patch7.13 KBjohnchque
#16 interdiff-3133439-14-16.txt1.94 KBjohnchque
#16 3133439-disable-add-above-modal-16.patch15.17 KBjohnchque
#16 3133439-disable-add-above-modal-16-NO-TESTS.patch7.16 KBjohnchque
#14 3133439-disable-add-above-modal-14.patch14.71 KBsasanikolic
#14 interdiff-3133439-11-14.txt2.08 KBsasanikolic
#11 interdiff-3133439-9-11.txt1.46 KBsasanikolic
#11 3133439-disable-add-above-modal-11.patch14.2 KBsasanikolic
#11 Add above visible for dropdown buttons.png79.11 KBsasanikolic
#9 interdiff-3133439-8-9.txt927 bytesjohnchque
#9 3133439-disable-add-above-modal-9.patch13.25 KBjohnchque
#9 3133439-disable-add-above-modal-9-NO-TESTS.patch5.24 KBjohnchque
#8 interdiff-3133439-5-8.txt8.38 KBjohnchque
#8 3133439-disable-add-above-modal-8.patch13.13 KBjohnchque
#5 interdiff-3133439-3-5.txt4.77 KBjohnchque
#5 3133439-disable-add-above-modal-5.patch4.55 KBjohnchque
#3 3133439-disable-add-above-modal-3.patch2.9 KBjohnchque
#2 3133439-disable-add-above-modal-2.patch772 bytesjohnchque
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yongt9412 created an issue. See original summary.

johnchque’s picture

Status: Active » Needs review
FileSize
772 bytes

This should work! Let's see about tests.

johnchque’s picture

So I think we can trigger the add button when we have it there. Had to move the delta hidden field so we can add above in other scenarios.

Status: Needs review » Needs work

The last submitted patch, 3: 3133439-disable-add-above-modal-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Status: Needs work » Needs review
FileSize
4.55 KB
4.77 KB

Finally found a way to move the delta textfield and updating the JS. :)

Status: Needs review » Needs work

The last submitted patch, 5: 3133439-disable-add-above-modal-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Issue tags: +Needs tests

We need to figure out why the existing test breaks and add test coverage for the extra mode where this is now supposed to work.

johnchque’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
13.13 KB
8.38 KB

It seems I just had to make some other small adjustments. As we are changing the delta class, that is what was causing the errors.

johnchque’s picture

Adding some comments here and there. Also added a no tests patch so I can use it in some projects.

The last submitted patch, 9: 3133439-disable-add-above-modal-9-NO-TESTS.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

sasanikolic’s picture

Here is the patch that fixes the form validation issue and page refresh issue. I also added a check for the number of buttons, I guess we want to trigger the click to add above only if there is only one button/paragraph that can be added? We're still left with the original issue I see, that the Add above button is still visible for multiple dropdown buttons and it doesn't work atm - shouldn't it be hidden?

Berdir’s picture

> Add above button is still visible for multiple dropdown buttons and it doesn't work atm - shouldn't it be hidden?

Correct, we only support the case with one button atm, so we need to check for that.

Berdir’s picture

Status: Needs review » Needs work
sasanikolic’s picture

Added the check for the number of buttons to the paragraphsAddAboveButton() function now. Should work as expected.

Status: Needs review » Needs work

The last submitted patch, 14: 3133439-disable-add-above-modal-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Hmm so I think we have to count the add buttons in a different way as the Add buttons in the modal have the same class that we are checking. I added an extra check for that, this should work. :)

sasanikolic’s picture

Status: Needs review » Needs work
  1. +++ b/js/paragraphs.add_above_button.js
    @@ -13,19 +13,25 @@
    +    var $add_more_wrapper = $button.closest('table').siblings('.clearfix').find('.paragraphs-add-wrapper');
    

    Can we add a more specific class to the buttons in the code where they get added (clearfix)? Reading this line is not very obvious what we are looking for. Also not very happy with the closest selector (table), but works in this case.

  2. +++ b/js/paragraphs.add_above_button.js
    @@ -38,12 +44,26 @@
    +          var $add_avobe = false;
    ...
    +            $add_avobe = true;
    ...
    +          if ($add_avobe) {
    

    Typos. :)

  3. +++ b/js/paragraphs.add_above_button.js
    @@ -38,12 +44,26 @@
    +          if ($add_more_wrapper.find('.paragraph-type-add-delta').hasClass('modal')) {
    ...
    +          else if ($add_more_wrapper.find('.field-add-more-submit').length === 1) {
    

    These two cases can be merged with a OR selector.

johnchque’s picture

Oh wow, my bad for the typo. Addressing suggestions. :) This should fix the tests and should make the class selector easier to understand.

The last submitted patch, 18: 3133439-disable-add-above-modal-18-NO-TESTS.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 18: 3133439-disable-add-above-modal-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

The last submitted patch, 21: 3133439-disable-add-above-modal-21-NO-TESTS.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

sasanikolic’s picture

Tested again locally with the latest patch. Seems to work fine now.

Maybe another code review from @Berdir before merging?

Berdir’s picture

Status: Needs review » Fixed

Thanks, committed.

  • Berdir committed 0716589 on 8.x-1.x authored by yongt9412
    Issue #3133439 by yongt9412, sasanikolic: Disable Add Above when Add...

Status: Fixed » Closed (fixed)

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