Support from Acquia helps fund testing for Drupal Acquia logo

Comments

toncic created an issue. See original summary.

Ginovski’s picture

Assigned: Unassigned » Ginovski
Status: Active » Needs review
FileSize
13.33 KB

We are missing:
-Text + Images
-Images + Text
-User
-Nested paragraph (but we have Container instead)

Adding patch with the 3 missing types (Text + Images, Images + Text, User) if we want to add them.

Primsi’s picture

Status: Needs review » Needs work

IMHO we don't need both T + I and I +T any more. I think having I + T is enough for now. I am also not sure if we want the User.

Ginovski’s picture

Status: Needs work » Needs review
FileSize
4.78 KB

Added only I+T

toncic’s picture

Status: Needs review » Needs work
+++ b/config/install/paragraphs.paragraphs_type.images_text.yml
@@ -0,0 +1,22 @@
+behavior_plugins:
+  grid_layout:
+    paragraph_reference_field: ''
+    available_grid_layouts: {  }
+  style:
+    group: ''
+  accordion:
+    paragraph_accordion_field: ''
+  slider:
+    field_name: ''
+    slick_slider: {  }
+  lockable: {  }
+  background:
+    background_image_field: ''
+  anchor: {  }

We can delete all of these.

Ginovski’s picture

Status: Needs work » Needs review
FileSize
704 bytes
4.47 KB

Addressed comment #5.

toncic’s picture

Status: Needs review » Needs work

Needs reroll, and try to install pc. We got some error about unmet dependencies.

Ginovski’s picture

Status: Needs work » Needs review
FileSize
4.57 KB

Rerolled, fixed fields.

miro_dietiker’s picture

The account paragraph type is an important concept to display an author profile.
Author profiles are known to be pretty specific though...

Still, i think it's important that the collection also contains examples of references to other entities so they can be placed in Paragraphs.

Ginovski’s picture

Added the user and added an example user in the paragraphs_collection_demo.

Primsi’s picture

Status: Needs review » Needs work
  1. +++ b/config/install/core.entity_view_display.paragraph.images_text.default.yml
    @@ -0,0 +1,35 @@
    +    label: above
    

    We probably don't need the label to be visible in any of the fields.

  2. +++ b/config/install/field.field.paragraph.images_text.paragraphs_images.yml
    @@ -0,0 +1,40 @@
    +  file_extensions: 'png gif jpg jpeg'
    

    svg?

  3. +++ b/config/install/field.field.paragraph.images_text.paragraphs_images.yml
    @@ -0,0 +1,40 @@
    +  alt_field_required: true
    

    Maybe we can leave the alt field as optional

  4. +++ b/config/install/field.field.paragraph.user.field_user.yml
    --- /dev/null
    +++ b/config/install/field.storage.paragraph.field_user.yml
    

    I think we are aiming for the field names to be prefixed by paragaraphs_ .... although we are not doing a good job at that.

  5. +++ b/modules/paragraphs_collection_demo/paragraphs_collection_demo.install
    @@ -200,6 +200,17 @@ function _paragraphs_collection_demo_create_demo_article() {
    +  $user = \Drupal::currentUser();
    

    We have a special page for fundamental types I think.

    Also do we want to add demo content for other fundamental types from this patch too?

Ginovski’s picture

Status: Needs work » Needs review
FileSize
8.03 KB
9.54 KB

Addressed #11.
1. Made the label hidden for the fields.
2. Added svg to the fiel extensions for images.
3. Made the alt field in the images optional
4. Changed fields names to be prefixed with paragraphs
5. Moved the user paragraph to the page for fundamental types and added an images+text paragraph content aswell.

Primsi’s picture

Status: Needs review » Needs work
  1. +++ b/config/install/field.storage.paragraph.paragraphs_user.yml
    @@ -0,0 +1,19 @@
    +cardinality: -1
    

    Let's not make this unlimited.

  2. +++ b/config/install/paragraphs.paragraphs_type.images_text.yml
    @@ -0,0 +1,7 @@
    +id: images_text
    +label: 'Images + Text'
    

    I think we are setting the field cardinalities to 1 in #2863568: Fix cardinality and label display of the fields in fundamental types so we might want to rename this to Image + Text

I also noticed that in demo the user field value is not set. Probably because you use current user which is 0 when installing via drush I think. Just user uid 1.

Ginovski’s picture

Status: Needs work » Needs review
FileSize
8.9 KB
9.74 KB

Addressed #13 comment.

  • Primsi committed bcc2eee on 8.x-1.x authored by Ginovski
    Issue #2859509 by Ginovski, toncic, Primsi: Identify which types we lost...
Primsi’s picture

Status: Needs review » Fixed

Committing this so that we can change the images field name in #2863568: Fix cardinality and label display of the fields in fundamental types. Thanks.

Status: Fixed » Closed (fixed)

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