Problem/Motivation

The expand all button changes to "Collapse all" if the first item is deleted.
However, clicking it doesn't change anything and i won't be able to "Expand all" ever again.

Proposed resolution

Skip deleted items and check the status of thw first that is visible.

User interface changes

Collapse / Expand all working as expected.

CommentFileSizeAuthor
#9 interdiff_6-9.txt1.11 KBsasanikolic
#9 paragraphs_cannot_expand_if_first_deleted_2956843_9.patch1.63 KBsasanikolic
#6 paragraphs_cannot_expand_if_first_deleted_2956843_6.patch1.53 KBsasanikolic
#2 paragraphs_cannot_expand_if_first_deleted_2956843.patch1.65 KBsasanikolic

Comments

miro_dietiker created an issue. See original summary.

sasanikolic’s picture

sasanikolic commented
StatusFileSize
new paragraphs_cannot_expand_if_first_deleted_2956843.patch1.65 KB

I added the if checks for the case when the first paragraph is deleted in a new function.

Here is the patch with the working solution.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Active » Needs review
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

The helper method code amount looks like we can loop over the paragraph items and simply pick the first non-deleted one at the same complexity.

toncic’s picture

toncic commented

+1 for to make this more dynamic, to do not depend only on first 2 items.

+++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
@@ -2445,6 +2444,32 @@ class ParagraphsWidget extends WidgetBase {
+  public function checkMode(array $field_state) {

Also we should try to find better name for this function. Something like 'getViewMode' or similar.

sasanikolic’s picture

sasanikolic commented
StatusFileSize
new paragraphs_cannot_expand_if_first_deleted_2956843_6.patch1.53 KB

Hi @miro_dietiker, I implemented your and @toncic's feedback in this patch.

sasanikolic’s picture

sasanikolic commented
Status: Needs work » Needs review
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
@@ -2535,6 +2534,32 @@ class ParagraphsWidget extends WidgetBase {
+  public function getViewMode(array $field_state) {

The term "View mode" has a clear meaning and is a different thing. That's why we name the key "edit_mode". Let's stick to that.

Also it is not returning a generic "edit mode". Each delta has its own edit mode. The method only determines the edit mode of the first delta by considering the default. We should name the interface accodringly.

sasanikolic’s picture

sasanikolic commented
StatusFileSize
new paragraphs_cannot_expand_if_first_deleted_2956843_9.patch1.63 KB
new interdiff_6-9.txt1.11 KB

I agree with the naming. Here is the patch with the improved naming and updated interface and comments.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs work » Needs review
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
@@ -2500,8 +2500,8 @@ class ParagraphsWidget extends WidgetBase {
-      $mode = isset($field_state['paragraphs'][0]['mode']) ? $field_state['paragraphs'][0]['mode'] : $this->settings['edit_mode'];

@@ -2535,6 +2535,32 @@ class ParagraphsWidget extends WidgetBase {
+  public function getFirstEditMode(array $field_state) {

Sorry for another round: To better support reuse we maybe should transform the method to "getFirstVisibleItemState" that just skips removed items, returning the item state as a whole.

The caller can then again apply the ternary.

I couldn't find any other occurence of ['paragraphs'][0] in Paragraphs itself anymore, but it's a common problem if you add other conditional actions. Maybe i have missed something...

I will also ping Berdir for quick feedback here to decide.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
@@ -2535,6 +2535,32 @@ class ParagraphsWidget extends WidgetBase {
+  public function getFirstEditMode(array $field_state) {
+    $mode = '';
+    if (isset($field_state['paragraphs'][0]['mode'])) {
+      foreach ($field_state['paragraphs'] as $paragraph) {
+        // Ignore deleted view mode and get first view mode.
+        if ($paragraph['mode'] !== 'remove') {
+          $mode = $paragraph['mode'];
+          break;
+        }
+      }
+    }
+    else {
+      $mode = $this->settings['edit_mode'];
+    }
+    return $mode;
+  }

A bit strange that we check for delta 0 in the condition but then loop over it, should be enough to just check that 'paragraphs' is defined?

Also, you can simplify it by just doing a return instead of setting a variable and doing a break.

Not sure if it's worth doing the suggested refactoring, I don't think it's such a common use case.