Hi,
First of all, thank you for this module.
I just have one observation. On the form for creating content with a Paragraphs field, the Paragraphs field doesn't appear the same way when there is already a paragraph :

Paragraphs with paragraph

and when there is no paragraph (Even without an asterisk for the required field) :

Paragraphs without paragraph

Would it be possible to have the same presentation for both as was done for the other fields (such as when there is already a paragraph)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Thib created an issue. See original summary.

johnchque’s picture

Version: 8.x-1.0 » 8.x-1.x-dev
Related issues: +#2831131: [META] Outperform field collections

This actually makes sense, not sure if this pattern is also seen in field_collection, but is worth discussing if we want to make this change.
Adding related issue.
If we will make this change, it will be in the dev branch.

Thib’s picture

Title: Paragraphs in content form » Displaying Paragraphs field in content form

On the second image (when there is no paragraph), the Paragraphs field appears to be a comment or annotation of the field above. It is difficult to identify it as a field in itself

Ginovski’s picture

Edit: I believe the required field part is covered in #2820502: Add form-required to required paragraph fields
Screenshots provided:
1. Before patch applied

2. After patch applied

Ginovski’s picture

Also, about the other part, in field collection, there is an instance of the field by default and it cannot be removed.
So maybe we can change to have the table-like header when there are no paragraphs fields present?

jonathanshaw’s picture

Title: Displaying Paragraphs field in content form » Field label is styled differently when field is empty
Category: Feature request » Bug report
johnchque’s picture

Category: Bug report » Task

I wouldn't say that is a bug, it was intended to be like that before. Let's discuss and/or implement.

mbovan’s picture

Issue tags: +drupalmountaincamp
johnchque’s picture

Was checking this, not sure if this would break some tests.

Status: Needs review » Needs work

The last submitted patch, 9: field_label_is_styled-2842094-9.patch, failed testing.

Thib’s picture

About #4, I don't know if the patch is committed in the 8.x-1.1 because it didn't work on my site

Antonnavi’s picture

Status: Needs work » Closed (duplicate)
Related issues: +#2820502: Add form-required to required paragraph fields

This issue was already fixed in: https://www.drupal.org/node/2820502

Thib’s picture

Status: Closed (duplicate) » Needs work

This is not the principal request of this issue : Field label is styled differently when field is empty...

johnchque’s picture

This also affects to the fact that when a field is single-valued then the label is not displayed.

johnchque’s picture

Not really sure if it is worth displaying the multivalue table always. This would mean that we need to check some hooks implemented earlier. :)
Order column displays nothing.
Rebasing patch.

Status: Needs review » Needs work

The last submitted patch, 15: field_label_is_styled-2842094-15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

miro_dietiker’s picture

Issue summary: View changes

It is not a Paragraphs problem that these two cases display different. Every multi- VS singlevalue field displays this way in Drupal Core.

However, the fact that these two cases are so much different are a problem for our widget.
For instance the Edit all / Collapse all / Drag & drop button can not be injected in the single value field.

Edit all / collapse all only affects its direct children, so we don't display them.

Drag and drop should in the long run also work across multiple host entity Paragraph fields, however it doesn't currently.
I was willing to drop the action button on single value fields, but the Paragraphs library uses a single value field and we put a container inside it to add multiple items. We want to have Drag & drop there.

Also to add the sticky header, it looks like these two cases add complexity.

There are more UX issues that try to deal with the effect that this situation creates.
Still not sure what is the best solution.

miro_dietiker’s picture

Priority: Normal » Major
Related issues: +#2899544: [META] Move Library to Paragraphs

Discussed and decided:
Changing the scope her a bit...

We want to display the table header always in the new widget.
It will help us to maintain consistency and multiple issues will be magically resolved. Most importantly the library UI will be fixed.

:-)

Let's get it done.

johnchque’s picture

Status: Needs work » Needs review
FileSize
4.82 KB
2.41 KB

Now showing the table header always, fixing tests. :)

miro_dietiker’s picture

Status: Needs review » Needs work

What a nice simplification. :-)

+++ b/paragraphs.module
@@ -341,7 +341,7 @@ function paragraphs_preprocess_field_multiple_value_form(&$variables) {
+  if ((isset($variables['element']['#allow_reference_changes']) && !$variables['element']['#allow_reference_changes']) || $variables['element']['#cardinality'] == 1 || count($variables['table']['#rows']) == 0) {

I'm confused about these conditions.

Can you explain a bit in a comment why they are checked? It feels like it is or even should be always true? ;-)

Berdir’s picture

FileSize
6.67 KB
+++ b/modules/paragraphs_demo/src/Tests/ParagraphsDemoTest.php
@@ -129,7 +129,7 @@ class ParagraphsDemoTest extends WebTestBase {
     $this->drupalGet('node/add/paragraphed_content_demo');
-    $this->assertRaw('<strong data-drupal-selector="edit-field-paragraphs-demo-title">Paragraphs</strong>', 'Field name is present on the page.');
+    $this->assertRaw('<th colspan="2" class="field-label"><h4 class="label">Paragraphs</h4>', 'Field name is present on the page.');
     $this->drupalPostForm(NULL, NULL, t('Add Text'));
     $this->assertNoRaw('<strong data-drupal-selector="edit-field-paragraphs-demo-title">Paragraphs</strong>', 'Field name for empty field is not present on the page.');

not sure if we really care about the th wrapper and the exact classes that it has? the h4 around it might be enough?

Also not sure about the two additional conditions, so yes, please document when they are needed. when the field is empty then there should never be a drag & drop handle?

Also, the empty text is very close to the add button, but I think it's already similar in HEAD for an empty paragraph. Now that we have the table header, I'm even wondering if we need that at all..

Also noticed that a single-cardinality field displays the drag handle as a simple dot, which seems pretty ok.

Berdir’s picture

+++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
@@ -1000,45 +1000,16 @@ class ParagraphsWidget extends WidgetBase {
+      '#description' => $this->realItemCount > 0 ? $description : $this->t('No @title added yet.', ['@title' => $this->getSetting('title')]),

ah, so you only show the description if there are paragraphs. That seems strange. As mentioned, we might want to just always show the description and maybe drop the no @title added yet part completely?

Miro, thoughts?

Berdir’s picture

Also surprised that we have no test fail on this, I thought we improved the description handling a while ago.. or maybe that never made it in.

miro_dietiker’s picture

Seems my post got lost. The empty text is confusing me since i took over maintenance and causted multiple times trouble as the "Paragraphs" concept typically only gains end-user visibility through the field name and the bundle name and both establish a consistent terminology. We should avoid promoting the entity type name (or the need to define a synonym for it) at all.

Also i don't see the empty text as a valuable accessibility feature. The Label [+ Row(s)] + Add XYZ button is a core pattern and we should return to it as much as possible.

So let's drop the empty text completely.

Berdir’s picture

> So let's drop the empty text completely.

And instead always display the description as I suggested?

miro_dietiker’s picture

Yeah. Always description.

I'm not a big fan of how Core outputs field descriptions (worst case is the fieldset description output after the fieldset)... But we can try to improve it and hopefully a Core improves it over time... (or contrib admin themes already do).

johnchque’s picture

Status: Needs work » Needs review
FileSize
4.98 KB
2.33 KB

Added a comment to explain the new conditions in the hook and changed to display the description always. Let's see if tests work.

Status: Needs review » Needs work

The last submitted patch, 27: field_label_is_styled-2842094-27.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.75 KB
4.34 KB

Updating tests, wanted to find a good way to check that the body is empty and head is present. :)

miro_dietiker’s picture

Status: Needs review » Fixed

Awwwesome, we dropped 4 spaghetti if-else conditions and 40 lines of related code with one default case (11 lines).

Committed.

miro_dietiker’s picture

Ah and for completeness... the library now looks beautiful with the table header, that made the two other issues fully fixed:
#2933516: The Drag & drop button is broken on single value Paragraph fields
#2941011: Library item paragraph field label not visible

More such issues please. :-)

Status: Fixed » Closed (fixed)

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