Split summary into content and behavior parts, show content/behavior only summary when switching between content and behaviors

+++ b/src/Entity/Paragraph.php
@@ -492,18 +493,23 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface {
 
-    $collapsed_summary_text = implode(', ', $summary);
-    return strip_tags($collapsed_summary_text);
+    $collapsed_summary_text = implode(', ', $summary_text);
+    return $collapsed_summary_text;
   }

so this comma is the problem?

Can we solve this with css? Either by using CSS to display a visual comma, or just use some other visual separator in case both elements are displayed.

I thought this issue is really only about setting an initial CSS state.
We are now half refactoring something that is not yet really where we want to be in the end.

+++ b/src/Entity/Paragraph.php
@@ -492,18 +493,23 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface {
       foreach ($paragraphs_type->getEnabledBehaviorPlugins() as $plugin_id => $plugin) {
...
+          $plugins_summary = array_merge($plugins_summary, $plugin_summary);
...
+      $text_plugins_summary = implode(', ', $plugins_summary);
+      $summary_text[] = '<span class="summary-plugin">' . $text_plugins_summary . '</span>';
...
+    $collapsed_summary_text = implode(', ', $summary_text);

In fact i think the goal should be to have a render element per plugin summary. Or one element that renders all behavior summary items at once. We would then drop all implodes by comma. Instead each behavior summary item would get some other visual separator and a class according to the behavior plugin name.

I previously thought it's an overkill, but we need this level of semantics in the summary to improve the summary styling for making it easier to read.

The best fitting element in standard design best practices was the material.io chips:
https://material.io/design/components/chips.html#
We need to be carefully to not make it look too much like a button.
We would either need an icon per behavior plugin or output them in two lines: behavior plugin name as overline and its status as regular chip content.

Do we really want to refactor this here?
I also couldn't find an existing open issue around that..
However chances are that before we implement this, someone needs to do a mockup and a screendesign.

What i don't understand is: If we have a toggle class like .content-active and .behavior-active.. Why do we additionally need to call .show() .hide() ? The visibility can already be managed through CSS based on the toggle class.

Or does .show() / .hide() also have some additional accessibility integration?

I did some code refactoring and fixes to John's patch.

1.) paragraphs-description contains general description styling
2.) add paragraphs-collapsed-description only when the paragraph is collapsed, so with that we can style it correctly
3.) when on content tab and paragraph is closed, display content + behavior summary, when it's collapsed, display only the behavior
4.) vice versa on the behavior tab
5.) change the comma to semicolon - i believe this is a better indicatior of separating two descriptions, although not optimal and we should find a better UX solution

Here are some screenshots.

Played a bit around and i think we should do this:

• Limit the summary to the current level only in case the Paragraph is expanded. Thus avoid duplications like behavior%20expanded.png
• Limit the summary text to the first line.
• Put Paragraphs behavior summary to the second line (if collapsed)
• We need a dom element per behavior summary item, not one for all combined. Maybe we even need a separate element for each the label to apply theming.
• Put Paragraphs behavior summary to the second line (if collapsed)
• Separate the behavior summary elements with padding, not a character like ";"
• Make the behavior summary text a bit smaller

Here some experiments but i'm not yet happy with it..

I addressed @miro_dietiker's feedback in this patch. We agreed to go with the 3rd solution, which looks the cleanest.

Above you accidentally dropped the comma in the content (missing!) but added the comma to the summary plugins (not needed).

I think i would also put the edit button to the baseline instead of vertically centering.
The grey BG definitively need a bit of padding.
I had a slightly bigger padding separating each behavior summary.

And yeah, there is good consistency now, but that IMHO results in too much uniformity.
In my tiny screens above i applied grey text color intentionally to the behavior summary text to lower the priority.

We need to check the output combined with icons - they can be left (e.g. change) and right (e.g. lock) of the summary!

Promoting priority as this is changing our summary DOM structure and we want this to be stable as soon as possible.
EDIT: And also because it is IMHO majorly improving the UI as a long awaited summary iteration.

I added the padding to the plugin name initially, but that resulted in misalignment with the content summary and was ugly. I can easily re-add that tho.

For example, see here:

The behavior summary display is wrong if the behavior summary text contains a ",". We should keep arrays and not do implode / explodes with intermediate strings...

Checked on mobile and we seem to have a bit too much vertical spacing. The extra large buttons add them, but not critical.

Checked a bit what patterns can be used here and ended up here:
https://material.io/design/components/lists.html#implementation

As you can see, they use often grey and a bit smaller font to make an item secondary.
The grey we have is still putting (too much) focus on the labels, that's why we tested further and ended up with this:

Making a label uppercase is also a common pattern with fieldsets (node edit sidebar, admin/config grouping).

I think this is the most balanced solution we have currently.

While on it, the child count badge (4) doesn't reserve a min-size, so horizontal indentation of the summary varies. Also it has some padding problem, making it look unbalanced.

Now you should remove the left padding of the second line to keep them aligned.

So, I found out that the cause of our issue lays in paragraphs_collection module, since we call $plugin->settingsSummary($this) from ParagraphsStylePlugin.php. What I did there, was to render the summary array instead of imploading it by comma, and that worked fine. I believe we should render the strings also in settingsSummary() in ParagraphsGridLayoutPlugin.php. We could also fix that in paragraphs, but it would just add complexity...

Not sure how to proceed there, since it's a separate module?

I did some small code fixes and fixed the styles.
I'm also attaching how the summary looks with my changes in paragraphs_collection.

For testing purposes, I also added a ","in the style plugin. Works fine.

Reverted back the array_merge change, as we needed it more plugins are available.

And here is the paragraphs_collection followup: https://www.drupal.org/project/paragraphs_collection/issues/3029996

1. +++ b/modules/paragraphs_library/tests/src/FunctionalJavascript/ParagraphsLibraryItemEntityBrowserTest.php
   @@ -167,12 +167,13 @@ JS;
        $save_button->press();
        $this->waitForAjaxToFinish();
   -    $this->assertRaw('class="paragraphs-collapsed-description">This is a reusable text UPDATED.');
   +    //debug($this->getSession()->getPage()->getContent());
   +    $this->assertRaw('class="paragraphs-description"><div class="paragraphs-summary-wrapper">This is a reusable text UPDATED.');
        $this->submitForm([], 'Save');
        // Edit the outside library item.
   

   debug leftover.

2. +++ b/src/Entity/Paragraph.php
   @@ -481,17 +482,43 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface {
   +    $content_text_summary = implode(', ', $summary);
   +    // Wrap only inner summary items.
   +    if ($content_text_summary != '') {
   +      if ($depth_limit !== 0) {
   +        $summary_text[] = '<div class="paragraphs-summary-wrapper">' . $content_text_summary . '</div>';;
   +      }
   +      else {
   +        $summary_text[] = '<span class="summary-content">' . $content_text_summary . '</span>';
   +      }
   +    }
   

   I think this is easier if we do "if ($summary) and then implode inside.

   I also think we should try to find a way to postpone the HTML wrappers until later. and not make them part of the array contents.

   For a start, I would nest the $summary array from the start, initialize it as $summary = ['content' => [], 'behaviors' => []], then put the parts inside those sub-keys.

   The depth check here also assumes that there is only 1 and 0, if you'd getSummary() with depth = 2, you'd get two div wrappers. so we might want to instead add an explicit nested = TRUE settingo the array and then do empty($options['nested']) instead of this.

3. +++ b/src/Entity/Paragraph.php
   @@ -481,17 +482,43 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface {
   +      if (!empty($plugins_summary)) {
   +        $wrap_plugins = function($plugin_summary_item) {
   +          $exploded_plugin = explode(': ', $plugin_summary_item);
   +          $exploded_plugin[0] = '<span class="summary-plugin-label">' . $exploded_plugin[0] . '</span>';
   +
   +          $wrapped_plugin = implode(' ', $exploded_plugin);
   +          return '<span class="summary-plugin">' . $wrapped_plugin . '</span>';
   +        };
   +
   +        $wrapped_plugins = array_map($wrap_plugins, $plugins_summary);
   +
   +        $text_plugins_summary = implode(' ', $wrapped_plugins);
   +        $summary_text[] = '<div class="paragraphs-plugin-wrapper">' . $text_plugins_summary . '</div>';
   +      }
   

   Instead of adding magic splits, why don't we add a explicit, optional support for nested arrays with label => value, we need to update the plugins anyway. Style would return something like this then:

   <?php
   [ ['label' => 'Group A', 'value' => 'Selected Style'], ['label' => 'Group B', 'value' => 'Another style']]

   Thinking more about this, we could then also make the top-level a render array with a template and a #content and #behaviors and a #theme => 'paragraphs_summary' as well as #nested. The whole HTML juggling could then be done in a single twig template.

   In there, we'd loop first over content items if there are any and second over behaviors, the content of these can then be checked with https://twig.symfony.com/doc/1.x/tests/iterable.html, and if an array, assume it has label/value keys, making the span structure explicit.

   not 100% sure yet how the nested part would work, as optimally, we'd only have a single-top level template, that would also deal with the nested stuff. We could add a new public method getContentSummaryTexts($options) that returns just an array of strings and merge them together.

I think it is important that it doesn't *break*, but we don't have to be very nice to them, if the summary item is not an array again, just set it as ['value' => $summary_part] and in twig, support that the label is empty by not printing the wrapper.

1. +++ b/src/Entity/Paragraph.php
   @@ -429,23 +429,23 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface {
            if ($file_summary != '') {
   -          $summary[] = $file_summary;
   +          $summary['contents'][] = $file_summary;
            }
   

   content is an uncountable noun, so just content, not contents: https://dictionary.cambridge.org/grammar/british-grammar/content-or-cont...

2. +++ b/src/Entity/Paragraph.php
   @@ -429,23 +429,23 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface {
            ]);
            if ($nested_summary != '') {
   -          $summary[] = $nested_summary;
   +          $summary = array_merge($summary, $nested_summary);
            }
   

   the != '' check is pretty strange, I don't think it is ever a string now? (just always return an array and you can remove the condition here.

   also, I think you can change getNestedSummary() to only return a flat list for the content and just merge that. because that array_merge() will not do what you think it does.

3. +++ b/src/Entity/Paragraph.php
   @@ -485,13 +485,17 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface {
    
   -    $collapsed_summary_text = implode(', ', $summary);
   -    return strip_tags($collapsed_summary_text);
   +    return $summary;
   

   this changes the return value of this method to return an array, which is causing a ton of test fails as you can see.

   We don't want to introduce an API change to the outside. You want to define the render array here, render it and then return the rendered string.

4. +++ b/src/Entity/Paragraph.php
   @@ -638,32 +642,31 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface {
              // Switch to the entity translation in the current context if exists.
              $entity = \Drupal::service('entity.repository')->getTranslationFromContext($entity, $this->activeLangcode);
   -          $summary[] = $entity->getSummary($options);
   +          $entity_summary = $entity->getSummary($options);
   +          foreach ($entity_summary['contents'] as $content) {
   +            $summary['contents'][] = $content;
   +          }
   +          foreach ($entity_summary['behaviors'] as $behaviors) {
   +            $summary['behaviors'][] = $behaviors;
   +          }
              $this->summaryCount++;
            }
   

   the problem then of course is this part.

   That is why I suggested to add a new public method that is named getContentSummaryParts() or so, that returns just the stuff that goes into 'content', then you call that here and not getSummary(). Basically, we'd refacter the part of getSummary() that sets all the keys in content into that new method.

   Another option would be to add a new setting that changes the return value, but that is generally considered to be bad design and an anti-pattern.

5. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
   @@ -834,9 +851,9 @@ class ParagraphsWidget extends WidgetBase {
                  $element['top']['summary']['fields_info'] = [
   -                '#markup' => $summary,
   -                '#prefix' => '<div class="paragraphs-collapsed-description">',
   -                '#suffix' => '</div>',
   +                '#theme' => 'paragraphs_summary',
   +                '#summary' => $summary,
   +                '#expanded' => FALSE,
                    '#access' => $paragraphs_entity->access('update') || $paragraphs_entity->access('view'),
   

   I see that defining it like this when we can is cleaner than having to go through a #markup again. Yet another idea would be this:

   We add getSummaryItems(), that is basically what getSummary() is now, and then re-add getSummary() that just calls getSummaryItems() and renders it into a string.

renderRoot() is definitely wrong, if anything then renderPlain()

Looks like you still have quite a few places where you now try to use the result of getSummary() as an array or so, either update that or use getSummaryItems() instead.

1. +++ b/modules/paragraphs_library/src/Entity/LibraryItem.php
   @@ -283,7 +283,7 @@ class LibraryItem extends EditorialContentEntityBase implements LibraryItemInter
       */
      protected static function buildLabel(ParagraphInterface $paragraph) {
        $summary = $paragraph->getSummary(['show_behavior_summary' => FALSE]);
   -    $summary = Unicode::truncate($summary, 50);
   +    $summary = Unicode::truncate(strip_tags($summary), 50);
        return $paragraph->getParagraphType()->label() . ': ' . $summary;
      }
   

   would it make sense to use getSummaryItems() here?

   It's not simpler, but we avoid having to render it with twig and then dropping the html tags again:

   $summary = $paragraph->getSummaryItems(['show_behavior_summary' => FALSE]);
       $summary = Unicode::truncate(implode(', ', $summary), 50);
   

2. +++ b/modules/paragraphs_library/tests/src/FunctionalJavascript/ParagraphsLibraryItemEntityBrowserTest.php
   @@ -167,12 +167,12 @@ JS;
        $save_button->press();
        $this->waitForAjaxToFinish();
   -    $this->assertRaw('class="paragraphs-collapsed-description">This is a reusable text UPDATED.');
   +    $this->assertRaw('class="paragraphs-description paragraphs-collapsed-description"><div class="paragraphs-content-wrapper"><span class="summary-content">This is a reusable text UPDATED.</span>');
        $this->submitForm([], 'Save');
   

   not sure if we need to be that specific. And since this is a FunctionalJavascript test, we could actually do this more easily with CSS selectors: $this->assertSession()->elementContains('css', '.paragraphs-description .summary-content', 'This is a reusable text UPDATED.');

3. +++ b/paragraphs.module
   @@ -590,3 +594,18 @@ function _paragraphs_migration_entity_type_adjust(array &$migration, $destinatio
   +  $variables['expanded'] = isset($variables['element']['#expanded']) ? $variables['element']['#expanded'] : FALSE;
   

   $variables['expanded'] = !empty(..) should give you the same result?

4. +++ b/src/Entity/Paragraph.php
   @@ -408,10 +409,54 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface {
   +   * @return array
   +   *   An keyed array with all content and behavior items of the summary.
   +   */
   +  protected function getContentSummaryItems($depth_limit = 1) {
        $summary = [];
   

   Now we have 3 different methods. I wondering if we can inline this back into getSummaryItems() now.

   Instead of using getContentSummaryItems(), we should be able to use getSummaryItems()['content']

5. +++ b/src/Entity/Paragraph.php
   @@ -638,32 +670,27 @@ class Paragraph extends ContentEntityBase implements ParagraphInterface {
              $entity = \Drupal::service('entity.repository')->getTranslationFromContext($entity, $this->activeLangcode);
   -          $summary[] = $entity->getSummary($options);
   +          $content_summary_items = $entity->getContentSummaryItems($options['depth_limit']);
   +          foreach ($content_summary_items as $content_summary_item) {
   +            $summary_content[] = $content_summary_item;
   +          }
              $this->summaryCount++;
            }
   

   Instead of the loop, you can just do $content_summary_items += $entity->getSummaryItems($options)['content'];

6. +++ b/src/ParagraphInterface.php
   @@ -47,11 +47,27 @@ interface ParagraphInterface extends ContentEntityInterface, EntityOwnerInterfac
   +   *
   +   * @return array
   +   *   An keyed array with all content and behavior items of the summary.
   +   */
   +  public function getSummaryItems(array $options = []);
   

   this could be a bit clearer on what array keys it actually returns, it's not behavior or behavior items but behaviors. something like "A list of summary items, grouped into the keys 'content' and 'behaviors'

7. +++ b/src/Plugin/Field/FieldFormatter/ParagraphsSummaryFormatter.php
   @@ -48,9 +47,7 @@ class ParagraphsSummaryFormatter extends EntityReferenceFormatterBase {
            $elements[$delta]['summary']['description'] = [
   -          '#markup' => $summary,
   -          '#prefix' => '<div class="paragraphs-collapsed-description">',
   -          '#suffix' => '</div>',
   +          '#markup' => $entity->getSummary()
            ];
   

   I think this could also use #theme paragraphs_summary, like the paragraphs widgets.

8. +++ b/src/Tests/Classic/ParagraphsEditModesTest.php
   @@ -85,7 +85,7 @@ class ParagraphsEditModesTest extends ParagraphsTestBase {
        $this->clickLink(t('Edit'));
   -    $this->assertRaw('<div class="paragraphs-collapsed-description">text_summary');
   

   these tests can't use the nicer asserts yet, but we could make it less specific and just look for text_summary. All we care about here IMHO is that it is the summary and it is less likely to break if we make changes to the template.

Great, committing. This is a big step in improving the summary.

Updating the issue title to reflect what we did here.