Overview

Let's start cleaning up the experience and remove the default content. This served us well while developing this, but since we have capabilities now to actually build a lot of the stuff by yourself, it doesn't seem necessary anymore.

Proposed resolution

  1. Remove the default value for the XB field (config/optional/field.field.node.article.field_xb_demo.yml)
  2. Remove the Hero field (config/optional/field.storage.node.field_hero.yml + config/optional/field.field.node.article.field_hero.yml)

User interface changes

Empty canvas when you start using XB!

CommentFileSizeAuthor
#30 XB-empty-canvas-to-start.gif809.66 KBwim leers
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

lauriii created an issue. See original summary.

lauriii’s picture

Title: Remove the default value and move components to a separate module » Remove the default value for the XB field and move components to a separate module
Issue summary: View changes
wim leers’s picture

Title: Remove the default value for the XB field and move components to a separate module » [PP-1] Remove the default value for the XB field and move components to a separate module
Status: Active » Postponed

This must happen after #3469436: Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property at least, and possibly more issues, to minimize disruption for in-progress MRs.

wim leers’s picture

Title: [PP-1] Remove the default value for the XB field and move components to a separate module » Remove the default value for the XB field and move components to a separate module
Priority: Major » Critical
Status: Postponed » Active
Issue tags: +Novice
Related issues: +#3469436: Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property
annmarysruthy’s picture

Assigned: Unassigned » annmarysruthy

deepakkm made their first commit to this issue’s fork.

annmarysruthy’s picture

Assigned: annmarysruthy » Unassigned
deepakkm’s picture

Assigned: Unassigned » deepakkm
Status: Active » Needs work
wim leers’s picture

Title: Remove the default value for the XB field and move components to a separate module » Update default config to make a fresh install result in an XB UI with an empty canvas
Issue summary: View changes
Issue tags: +Needs screenshots

Talked to @lauriii, narrowing the scope, to the part that is actually important for #3454094: Milestone 0.1.0: Experience Builder Demo 👍

wim leers’s picture

wim leers’s picture

tedbow’s picture

Assigned: deepakkm » tedbow

@deepakkm thanks the work on this. Since it past your end of day and this is important for #3454094: Milestone 0.1.0: Experience Builder Demo I am going to try move this forward

tedbow changed the visibility of the branch 3472299-remove-default-xb-field to hidden.

tedbow changed the visibility of the branch 3472299-remove-default-xb-field to active.

tedbow’s picture

I have an idea I am working to leave the test coverage mostly the same so I am going to leave this assigned to me

wim leers’s picture

If I do a fresh install of XB using this MR (and rebuilt the UI — I tested commit 4ce1b00bc8d2a41309ffcea12ae62b4a27a2845e of this MR), then I do start with an empty canvas. 🥳

Unfortunately, I cannot place any component onto the canvas without fatal errors, except the sole propless component (Druplicon)

🚨 Surprisingly, @jessebaker and @shyam_bhatt did not run into the below that when they created #3473761: Render a placeholder DIV inside empty Regions/Slots in the preview, where they manually emptied the canvas. So their starting point was different. I bet there's an (initialization) edge case there 🤓

But there’s something odd going on with addNewComponentToLayout() in the UI: when dragging the Heading component onto the empty canvas, the UI correctly initializes initialData (I just stepped through it), and hence

      dispatch(
        insertNode({
          to: payload.to,
          newNode: {
            children,
            nodeType: 'component',
            type: payload.newNode,
          },
          model: initialData,
        }),
      );

has the correct model, yet somehow that information is lost by the time the request is sent to /api/preview/node/1:

{
  "layout": {
    "uuid": "root",
    "nodeType": "root",
    "name": "root",
    "children": [
      {
        "children": [],
        "nodeType": "component",
        "type": "experience_builder:heading",
        "uuid": "611ba3f1-e3c8-4364-a512-44dd83501f82"
      }
    ]
  },
  "model": []
}
wim leers’s picture

To make TranslationTest pass, we need it to not rely on field_hero anymore. So, changes to the default

  • removed: {"uuid":"dynamic-image-udf7d","component":"experience_builder:image"},
  • replaced:
    {"uuid": "dynamic-image-static-imageStyle-something7d","component": "experience_builder:image"}
    with:
    {"uuid":"static-heading-some-uuid","component":"experience_builder:heading"}
    + as prop value:
    "static-heading-some-uuid":{"text":{"sourceType":"static:field_item:string","value":"hello, world!","expression":"ℹ︎string␟value"},"element":{"sourceType":"static:field_item:list_string","value":"h3","expression":"ℹ︎list_string␟value","sourceTypeSettings":{"storage":{"allowed_values":[{"value":"h1","label":"h1"},{"value":"h2","label":"h2"},{"value":"h3","label":"h3"},{"value":"h4","label":"h4"},{"value":"h5","label":"h5"},{"value":"h6","label":"h6"},{"value":"div","label":"div"}]}}}}
    
  • replaced the cta1href prop value for
    {"sourceType":"dynamic","expression":"ℹ︎␜entity:node:article␝field_hero␞␟entity␜␜entity:file␝uri␞␟value"}}
    with
    {"sourceType":"static:field_item:link","value":{"uri":"https:\/\/drupal.org","title":null,"options":[]},"expression":"ℹ︎link␟uri"}}
wim leers’s picture

For the Cypress E2E tests, I recommend:

  • switching back to 0.x
  • apply
    diff --git a/tests/src/TestSite/XBTestSetup.php b/tests/src/TestSite/XBTestSetup.php
    index 87680653..9c4409a0 100644
    --- a/tests/src/TestSite/XBTestSetup.php
    +++ b/tests/src/TestSite/XBTestSetup.php
    @@ -75,6 +75,7 @@ class XBTestSetup implements TestSetupInterface {
           'title' => 'XB Needs This For The Time Being',
         ]);
         $node->save();
    +    var_dump($node->get('field_xb_test')[0]->toArray());
     
         $xb_role = Role::create([
           'id' => 'xb',
    
  • execute any Cypress test, observe the dumped output
  • copy/paste that dumped output into the Node::create() call in XBTestSetup
  • … that should keep all Cypress tests running exactly as-is, with zero changes
jessebaker’s picture

RE #17

It seems that the initial request for the page data when loading the page made to /api/layout/node/1 is returning

{
    "layout": {
        "uuid": "root",
        "nodeType": "root",
        "name": "root",
        "children": []
    },
    "model": []
}

The important part of that is that "model": [] here is coming back as a JS Array. When the model has content, the request returns model as a JS Object ("model": {}).

If required, we can probably re-shape the data to make it work on the front end in this case, but I think the better solution would be to solve it on the back end.

tedbow’s picture

tedbow’s picture

Assigned: tedbow » wim leers
Status: Needs work » Reviewed & tested by the community
tedbow’s picture

Status: Reviewed & tested by the community » Needs review

whoops

danielveza’s picture

Left a couple of tiny comments on the MR.

The IS mentions updating CONTRIBUTING.md, I've checked it in the merge branch and I don't think anything needs to be updated :)

wim leers’s picture

Merged in upstream changes that #3474226: Make Media Library a dependency landed … curious to see if this still passes 🤓

wim leers’s picture

Assigned: wim leers » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

#24++ :)

tedbow’s picture

Status: Reviewed & tested by the community » Needs work
  1. re #24
    From contributing.md

    5. Browse to `/node/add/article` — you'll see a `🪄 XB Demo ✨` field. Don't touch that — just enter a title for the article and hit save: a component is rendered using the article title 🤓
    I think we do need to update this.
    The component is not render with title by default now, and you don't actually see the field in /node/add/article(maybe that was pre-existing).

  2. Manually testing, I did a fresh install and now I get error trying add a component.

    TypeError: array_diff_key(): Argument #1 ($array) must be of type array, null given in array_diff_key() (line 42 of /Users/ted.bowman/sites/exp-d-core/modules/contrib/experience_builder/src/Controller/SdcController.php).

    #0 /Users/ted.bowman/sites/exp-d-core/modules/contrib/experience_builder/src/Controller/SdcController.php(42): array_diff_key(NULL, Array)
    #1 /Users/ted.bowman/sites/exp-d-core/modules/contrib/experience_builder/src/Plugin/DataType/ComponentTreeHydrated.php(60): Drupal\experience_builder\Controller\HardcodedPropsComponentTreeItem->resolveComponentProps('353413ac-18b1-4...')
    #2 /Users/ted.bowman/sites/exp-d-core/modules/contrib/experience_builder/src/Plugin/DataType/ComponentTreeHydrated.php(121): Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated->getValue()
    #3 /Users/ted.bowman/sites/exp-d-core/modules/contrib/experience_builder/src/Plugin/Field/FieldType/ComponentTreeItem.php(309): Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated->toRenderable()
    #4 /Users/ted.bowman/sites/exp-d-core/modules/contrib/experience_builder/src/Controller/SdcController.php(389): Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem->toRenderable()

    I switched by to https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... and it worked again.

    Maybe something changed in 0.x and the merged change caused this?

wim leers’s picture

#27.2: thanks, that's what I was gonna do next before merging, and why I didn't merge rapidly before the personal stuff I left for at #26 😅

That's exactly the problem I was running into I ran into in #17.

tedbow’s picture

Assigned: Unassigned » wim leers
Status: Needs work » Needs review
wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots
StatusFileSize
new809.66 KB

Manually tested, works well!

wim leers’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

  • wim leers committed 8d181e97 on 0.x authored by deepakkm
    Issue #3472299 by deepakkm, wim leers, tedbow, danielveza, jessebaker:...

Status: Fixed » Closed (fixed)

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