Variants - inheriting data from the base

I created an Organism that includes an array of a single molecule.
http://mayflower.digital.mass.gov/?p=organisms-contact-list

I then created a variant of that Pattern using the contact-list~single-item.json method to show how the component should be rendered if there is only one molecule in the array.
http://mayflower.digital.mass.gov/?p=organisms-contact-list-single-item

After upgrading our site to the Drupal Pattern Lab (massgov/mayflower#488), the Variant began showing two items instead of one. The first item was being displayed correctly, but the second item, which shouldn't have been there, was coming from the base json file.

I'm not sure why this is happening, but it seems like a bug.

{
  "contactList": {
    "compHeading": {
      "title": "Contacts",
      "sub": "",
      "color": "",
      "id": "contacts",
      "centered": ""
    },

    "contacts":[{
      "accordion": false,
      "isExpanded": false,
      "subTitle": {
        "href":"",
        "text": "Department of Conservation and Recreation"
      },
      "groups": [{
        "icon": "@atoms/05-icons/svg-phone.twig",
        "name": "Phone",
        "items": [{
          "type": "phone",
          "property": "",
          "label":"Main:",
          "link": {
            "href": "+14134994262",
            "text": "(413) 499-4262",
            "info": "",
            "property": ""
          },
          "details":"This line is open from 9:00 a.m. - 5:00 p.m., M-F"
        },{
          "type": "phone",
          "property": "",
          "label":"Toll Free:",
          "link":{
            "href": "+18004324321",
            "text": "(800) 432-4321",
            "info": "",
            "property": ""
          },
          "details":"This line is open from 9:00 a.m. - 5:00 p.m., M-F"
        },{
          "type": "phone",
          "property": "",
          "label":"TTY:",
          "link": {
            "href":"+16175557777",
            "text": "(617) 555-7777",
            "info": "",
            "property": ""
          },
          "details":"For those who need accessible services"
        },{
          "type": "phone",
          "property": "",
          "label":"Voice Relay:",
          "link":{
            "href":"16175555255",
            "text": "(617) 555-5255",
            "info": "",
            "property": ""
          },
          "details":""
        }]
      },{
        "icon": "@atoms/05-icons/svg-laptop.twig",
        "name": "Online",
        "hidden": "",
        "items": [{
          "type": "email",
          "property": "",
          "label":"Email:",
          "link": {
            "href":"email@email.com",
            "text": "email@email.com",
            "info": "",
            "property": ""
          },
          "details":""
        },{
          "type": "online",
          "property": "",
          "label":"Web:",
          "link": {
            "href":"#",
            "text": "Contact Form",
            "info": "",
            "property": ""
          },
          "details":""
        }]
      },{
        "icon": "@atoms/05-icons/svg-fax-icon.twig",
        "name": "Fax",
        "items": [{
          "type": "fax",
          "property": "",
          "label":"Main:",
          "link": {
            "href":"+14134994262",
            "text": "(413) 499-4262",
            "info": "",
            "property": ""
          },
          "details":"Please allow 24 hours for a response"
        }]
      },{
        "icon": "@atoms/05-icons/svg-marker.twig",
        "name": "Address",
        "items": [{
          "type": "address",
          "property": "",
          "label": "",
          "address": "555 Park Drive\nBoston, MA 02231",
          "link": {
            "href": "https://www.google.com/maps/place/555+Park+Dr,+Boston,+MA+02215",
            "text": "directions",
            "info": "",
            "property": ""
          },
          "details": ""
        }]
      }]
    }]
  }
}

@dmolsen

Hey @legostud - so after doing quite a bit of digging (and a whole lot of experimenting with the logic behind how the psuedo-pattern data gets merged together in the vendor/pattern-lab/core/src/PatternLab/PatternData/Rules/PseudoPatternRule.php file) I think I better understand the problem here... but I'm not sure I necessarily have a "solution" per say.

Lemme try to break this down a bit.

1. Regarding this quirk showing up w/ the PR from a couple weeks ago updating Mayflower to the Drupal Pattern Lab Fork:

From what I can tell, this issue with some of the data getting duplicated in the pseudo pattern stems from this commit from @dmolsen from a little over a year ago that changed the default data merging behavior for pseudo-patterns in Pattern Lab from a simple array_merge to a array_replace_recursive merge.

Sure enough, if I revert those two lines locally, the single item variant renders how I believe you'd be expecting it to:

2. **That said, this brings up the important question, what's the correct behavior here?**

Let's pretend for a moment that this single item variant didn't/shouldn't want to manually re-include the "Contacts" title by default (after all, that's sort of the whole point of having inherited data).

Rather than setting the contactList.compHeading.title value to an empty string, let's assume we just don't include that value in our json file and re-generate Pattern Lab.

contactList": {
    "example-prop": "contact list",
    "viewSpecific": false,
    "compHeading": {
      "sub": "",
      "color": "",
      "id": "contacts",
      "centered": ""
    },
    ...
}

Instead of inheriting the "Contact Us" text from the original pattern (or some other data property we'd want to trickle down into variants so we don't need to duplicate the data structure over and over again, instead the title gets overwritten by the other compHeading data values and we're left without a title...

vs. the update from July of last year which replaced array_merge with array_replace_recursive

which properly inherit the default title set in the base pattern. If I'd have to guess, this is probably why the data across the board in PL is replaced recursively vs merged.

So.... what's the correct behavior? ¯_(ツ)_/¯

3. All that said, I think there's a couple options we could go from here.

My personal opinion would be to go in one of two different directions:

Option 1: Keep the default merging behavior, but take a slightly different approach to generating variants.

@EvanLovely had some awesome slides during his Drupalcon Baltomore 2017 presentation on working with Pinterest on their component library and had this one slide in particular that really stood out:

Essentially, each pattern should have a vanilla pristine underscored version that both PL and Drupal reference and pass in the data per usual via separate twig file variations.

So for this example, you could simply have a _contact-list.twig template that gets included or extended via a separate contact-list--single-item.twig file to get all the benefits of the array_merge + array_replace_recursive:

{% extends "@organisms/by-author/_contact-list.twig" %}

OR

{% include "@organisms/by-author/_contact-list.twig" %}

Option 2: Allow for Default PL Rules to Be Configurable

Alternatively, if we went down this route, we could allow PL users to pick and choose which is the right behavior for them (while still providing an out of the box default set of rules to start off with).

Side note: I'm sad that the slides for Evan's presentation didn't get included in the YouTube video. A PDF of the slides would be amazing....

CC @EvanLovely, @aleksip, or @jesconstantine -- any thoughts?

@sghoweri

@sghoweri - In your example of omitting "title", the pattern is still rendering (see the green line) just without any text. All of the html is still there.

In the example I'm showing, the second item is an accordion that expands to show another element.

The question I still have is that I have reset the "contactList.contacts" variable from having two values to having one value in it array, as such it should render one element. I could accept it rendering two elements if I omitted the contacts variable, because I then wouldn't be overriding the default contacts variable.

Salem, I'm on vacation this week, but I wanted to drop in the link to my slides from the presentation. I'm super bummed they messed up the recording and plan to fix it myself eventually.

http://www.evanlovely.com/presentations/drupalcon-2017-pinterest-pattern-lab/DrupalCon_2017_-_Pinterest_Pattern_Lab.pdf

Those slides provided some insight into this problem. Base on what I'm seeing in Evans slides, the pattern for creating variants is different than how I've been doing them. In these examples, all of the data passed is done using 'with' so a variant is created as a new twig file with a different data object passed down.

For my project, I'm using pseudo patterns, which uses multiple json files for one twig file.
http://patternlab.io/docs/pattern-pseudo-patterns.html

found another broken pattern (two column variant). Similar issue with the array being extended.

@EvanLovely

Hey thanks @EvanLovely - enjoy your time off!

Reverting Pattern Lab psuedo-pattern rules to use original merge func…

Workaround to address unexpected Pattern Lab Psuedo-Pattern Data Inhe…

Master 5.6.1 to Dev (#557)

DP-4323 Pattern Docs at 75% (#545)

Navigation Menu

Repository navigation

Variants - inheriting data from the base #12

1. Regarding this quirk showing up w/ the PR from a couple weeks ago updating Mayflower to the Drupal Pattern Lab Fork:

2. **That said, this brings up the important question, what's the correct behavior here?**

3. All that said, I think there's a couple options we could go from here.

Option 1: Keep the default merging behavior, but take a slightly different approach to generating variants.

Option 2: Allow for Default PL Rules to Be Configurable

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Variants - inheriting data from the base #12

Description

Activity

sghoweri commented on Jul 23, 2017

1. Regarding this quirk showing up w/ the PR from a couple weeks ago updating Mayflower to the Drupal Pattern Lab Fork:

2. That said, this brings up the important question, what's the correct behavior here?

3. All that said, I think there's a couple options we could go from here.

Option 1: Keep the default merging behavior, but take a slightly different approach to generating variants.

Option 2: Allow for Default PL Rules to Be Configurable

gender404 commented on Jul 24, 2017

EvanLovely commented on Jul 24, 2017

gender404 commented on Jul 24, 2017

gender404 commented on Jul 24, 2017

sghoweri commented on Jul 25, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

2. **That said, this brings up the important question, what's the correct behavior here?**