Problem/Motivation

When using the styles in the node edit form we can get redundant labels that can just clutter the interface since the context where the plugin is used is well understood, however we might need a different label in the styles overview to clarify what type of style is it.
Also, when using the Style system we usually get some names that are inconsistent with lowercase and uppercase characters.

Proposed resolution

Add a label key to the style group definition that will be displayed to the user.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#18 interdiff-3025786-16-18.txt674 bytesjohnchque
#18 label_group_definition-3025786-18.patch9.45 KBjohnchque
#16 interdiff-3025786-14-16.txt4.83 KBjohnchque
#16 label_group_definition-3025786-16.patch9.5 KBjohnchque
#14 interdiff-3025786-12-14.txt572 bytesjohnchque
#14 label_group_definition-3025786-14.patch9.79 KBjohnchque
#12 interdiff-3025786-11-12.txt991 bytesjohnchque
#12 label_group_definition-3025786-12.patch9.34 KBjohnchque
#11 interdiff-3025786-10-11.txt667 bytesjohnchque
#11 label_group_definition-3025786-11.patch9.34 KBjohnchque
#10 interdiff-3025786-9-10.txt2.34 KBjohnchque
#10 label_group_definition-3025786-10.patch8.62 KBjohnchque
#9 interdiff-3025786-8-9.txt2.64 KBjohnchque
#9 label_group_definition-3025786-9.patch8.49 KBjohnchque
#8 interdiff-3025786-7-8.txt362 bytesjohnchque
#8 label_group_definition-3025786-8.patch8.05 KBjohnchque
#7 interdiff-3025786-5-7.txt4.51 KBjohnchque
#7 label_group_definition-3025786-7.patch8.61 KBjohnchque
#5 interdiff-3025786-3-5.txt811 bytesjohnchque
#5 label_group_definition-3025786-5.patch7.25 KBjohnchque
#3 label_group_definition-3025786-3.patch6.62 KBjohnchque
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yongt9412 created an issue. See original summary.

johnchque’s picture

Issue summary: View changes
johnchque’s picture

Status: Active » Needs review
FileSize
6.62 KB

This should work. :)

Status: Needs review » Needs work

The last submitted patch, 3: label_group_definition-3025786-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
7.25 KB
811 bytes

Using the new short label in the summary now.

johnchque’s picture

Status: Needs review » Needs work

Wait no, this is not working properly.

johnchque’s picture

Status: Needs work » Needs review
FileSize
8.61 KB
4.51 KB

OK, the previous patch was not working because the first time that we loaded the styles we loaded them with either the label or the context label but not both. Now we have both and I added some methods for working with these. :)

johnchque’s picture

Removing changes in demo.

johnchque’s picture

So we can avoid displaying the Style in the behavior form if we have a context label for a group. :)

johnchque’s picture

Ok, adding some comments and refactoring some lines. :)

johnchque’s picture

We need to update the usage of this method.

johnchque’s picture

Updating Style to be only lowercase. :)

Status: Needs review » Needs work

The last submitted patch, 12: label_group_definition-3025786-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Status: Needs work » Needs review
FileSize
9.79 KB
572 bytes

Fixing tests. :)

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/modules/paragraphs_collection_test/paragraphs_collection_test.paragraphs.style_group.yml
    @@ -2,6 +2,7 @@ regular_test_group:
     bold_test_group:
       label: 'Bold Test Group'
    +  context_label: 'Bold CONTEXT'
     italic_test_group:
    

    Hm, not sure about the key. Maybe widget_label?

  2. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
    @@ -115,9 +115,14 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
    +        $title = $this->t('%group style', ['%group' => $this->yamlStyleDiscovery->getGroupLabel($group_id)]);
    +        // If there is a context label use it without adding a "Style" suffix.
    +        if ($group_label = $this->yamlStyleDiscovery->getGroupContextLabel($group_id)) {
    +          $title = $this->t('%group', ['%group' => $group_label]);
    +        }
    

    this means you're doing the work twice, use an if/else instead.

  3. +++ b/src/Plugin/paragraphs/Behavior/ParagraphsStylePlugin.php
    @@ -290,8 +295,12 @@ class ParagraphsStylePlugin extends ParagraphsBehaviorBase implements ContainerF
    +          // If there is not a context label, use the group label instead.
    +          if (!$group = $this->yamlStyleDiscovery->getGroupContextLabel($group_id)) {
    +            $group = $this->yamlStyleDiscovery->getGroupLabel($group_id);
    +          }
               $summary[] = $this->t('@group: @style', [
    -            '@group' => $this->yamlStyleDiscovery->getGroupLabel($group_id),
    +            '@group' => $group,
    

    hm, above you call it $title, here $group and the key/method is label. I'd use $label in both.

    Having to duplicate this logic if trying multiple labels makes me sad, but it's tricky as above we have it combined with " style" and here we don't. But actually, that is kind of strange, as the shortened label here without style suffix possibly not enough context.

    Wondering if we should just drop the style sufffix completely and simply add it to the group labels where we want to have it. Downside is that we would have a bit of duplication then in places like the style overview. Or we would alternatively add " style" also here, then we could do getGroupWidgetLabel() that would return the widget_label or fallback to "$label style"?

  4. +++ b/src/StyleDiscovery.php
    @@ -274,10 +277,33 @@ class StyleDiscovery implements StyleDiscoveryInterface {
    +    if (in_array($group_id, array_keys($groups))) {
    +      return $groups[$group_id]['context_label'];
         }
         return NULL;
    

    Or you could just do a isset($groups[$group_id]) ;)

johnchque’s picture

Status: Needs work » Needs review
FileSize
9.5 KB
4.83 KB

Addressed all comments above.

About 3. I decided to include the style suffix straight in the method getGroupWidgetLabel becuase I believe that we should keep consistent the labels between widget modes. :)

Status: Needs review » Needs work

The last submitted patch, 16: label_group_definition-3025786-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Status: Needs work » Needs review
FileSize
9.45 KB
674 bytes

This should fix the test. Thanks @Berdir. :)

  • Berdir committed 724c485 on 8.x-1.x authored by yongt9412
    Issue #3025786 by yongt9412: Add label key to style group definition
    
Berdir’s picture

Status: Needs review » Fixed

Thanks. Committed.

Status: Fixed » Closed (fixed)

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