Impossible to drop a component into an empty column of the two

🐛 Cannot place or drag child components Fixed landed.

Wim Leers → credited jessebaker → .

Paired with @jessebaker, he let me know what's needed:

1. slots without components placed in them MUST contain <div class="sortable-list" data-xb-uuid="%s" data-xb-type="%s"></div> for it to be presented as a slot in the UI
2. slots without components placed in them BUT with default content (i.e. some markup) must wrap that default content in that same markup, to allow overriding that default content

On it.

SDC default slot value woes

Turns out that SDC does not support:

1. setting [#slots][$slot_name] = [] → you'll get: Drupal\Core\Render\Component\Exception\InvalidComponentDataException: Unable to render component "experience_builder:two_column". A render array or a scalar is expected for the slot "column_one" when using the render element with the "#slots" property in Drupal\Core\Render\Element\ComponentElement->generateComponentTemplate() (line 118 of core/lib/Drupal/Core/Render/Element/ComponentElement.php).
2. setting [$slots[$slot_name] = [#prefix => …, #suffix => …] → you'll get only the prefix + suffix

But it does support not specifying any value at all for a slot, then it'll happily use the default.

IOW: I cannot wrap the default value of a slot (using #prefix + #suffix) — doing so causes the default value to disappear 😬

Investigation

So I investigated, to understand how to respect the SDC metadata for the two-column component that @ctrlADel added in 📌 Introduce an example set of representative SDC components; transition from "component list" to "component tree" Fixed :

  column_one:
    title: Column One
    description: The contents of the first column.
    # @todo can_be_dropzone: true
    examples:
      - <p>This is column 1 content</p>
    # @todo this should be a dropzone
  column_two:
    title: Column Two
    description: The contents of the second column.
    # @todo can_be_dropzone: true
    examples:
      - <p>This is column 2 content</p>
    # @todo this should be a dropzone

And discovered that two-column.twig du:

<div {{ attributes }}>
    {% if block('column_one') %}
      <div class="column-one width-{{ width }}">
        {% block column_one %}
          The contents of the one column.
        {% endblock %}
      </div>
    {% endif %}

    {% if block('column_two') %}
      <div class="column-two width-{{ 100-width }}">
        {% block column_two %}
          The contents of the two column.
        {% endblock %}
      </div>
    {% endif %}
</div>

… as you can see, the Twig template duplicates the sample content. That is where the default was coming from: from the Twig template not the SDC metadata! 😅

Now that I understand that, I can continue figuring out a solution.

TIL #slotsAlter, while discovering #5.

🐛 The test coverage in \Drupal\KernelTests\Components\ComponentRenderTest::checkRenderElementAlters() is incorrect. It suggests you can choose which slot to alter, but you cannot: it alters the *value* of one slot at a time. (Unlike #propsAlter, which alters all props at the same time, just as the name + test logic suggests.)

Hopefully that won't block this issue; because fixing that core bug would come with nasty BC break consequences.

Upstream core bug reported for #6: 🐛 `#propsAlter` alters all props simultaneously, `#slotsAlter` does not, despite its name and test coverage Active .

I've given @balintbrews as much context about this issue as I can so hopefully he can assist with anything you need next week.

t

This looks great, and it's very close! I noticed that you're able to drag the default content, and place it even outside of its slot.

I pushed a fix for that, but we need to be able to target the default content somehow, so I propose that we wrap it in markup with a class named xb-internal-component-example. (Feel free to rename it if you have a better idea or if you think we can simplify it.) Would you mind adjusting the rendering logic accordingly?

I also added code to prevent inserting a component above the example — this way we avoid having the example content move up and down, which may suggests that it stays in place even after a component is added.

Great observation!

If I hack that together without writing code:

diff --git a/components/containers/two_column/two_column.component.yml b/components/containers/two_column/two_column.component.yml
index 4ba49c11..149b9bb2 100644
--- a/components/containers/two_column/two_column.component.yml
+++ b/components/containers/two_column/two_column.component.yml
@@ -18,7 +18,7 @@ slots:
     title: Column One
     description: The contents of the first column.
     examples:
-      - <p>This is column 1 content</p>
+      - <p class="xb-internal-component-example">This is column 1 content</p>
   column_two:
     title: Column Two
     description: The contents of the second column.

… then it looks like this:

… which makes me wonder: wouldn't it be better if:

• the entire column becomes the drop target? (i.e. the existing markup is overlaid with the gray diagonal lines that indicate a drop target)
• … which removes the need for the before vs after distinction
• … and would remove the "jumping" of the component just after dropping it

Assuming you'd agree that's a good idea, I implemented something different instead: annotated the div.sortable-list wrapper with data-xb-slot-is-empty when it contains no components:

WDYT? 😊

I like that idea! I played exactly with that last night after pushing my code, but deemed it too complex to do. I took a fresh look today and managed to implement it.

Towards the end of the gif you can see that we could tweak the sensitivity of the drop target boundaries, but I thought that could be part of a more comprehensive round of UI improvements, so I didn't spend time with that. The same goes for making the entire slot the "ghost element" and not only the fixed size that's used everywhere.

What do you think?

+1!

But currently the display: none is causing a somewhat jarring experience IMHO: it causes the drop zone to disappear mid-drag-and-drop, which even makes it unreachable if you drop it back in the original location:

Okay, this does need more work. I can see two problems:

1. When dragging an item over a slot, it's very easy to trigger another slot's drop zone. Not only it is too easy, it's unexpected, and we should probably consider it a bug as opposed to how I generously described it as something to tweak. 🙂
2. While there is code in my latest commit that restores the visibility of the examples, it clearly doesn't always work. I couldn't quite figure out how to reproduce, but I was able to reproduce by playing around, and it's also visible on your screen capture.

The first problem can be somewhat temporarily mitigated by doing what I did initially: leaving the example in place and only allowing to drop something below it. If we do this, the second problem goes away for now. Then we can open a follow-up issue to address the problem properly, then re-introduce hiding the example content.

Or we can try to address everything in this issue. I'm happy either way! @Wim Leers, let me know how you prefer to proceed.

1. So: when needing to "fly over" some slots to get to the "intended destination slot", we trigger unintended things. Is that what you mean?

   If correct: I think that could be a fine follow-up issue. That's a weakness that this MR reveals, it doesn't introduce it?

   i.e. I expected to be able to drag onto any part of the left columnin #18, instead of only the comparatively tiny target at the top.

2. AFAICT it's really just the need to revert the display: none if the drag-and-drop action was canceled (i.e. if the dragged sortable is restored to its original position).

1. Both are correct: good rewording of the problem, and yes, this is something the changes emphasize, but it's certainly not new. I'll create a follow-up issue. I will mention increasing the drop target for slots — I already had some code experimenting with that. Also worth mentioning that 🐛 Retain the space consumed by a component when it's being dragged Fixed is also a nice improvement over what we're experiencing here.
2. Nice! This is something I somehow missed testing! Based on your description I was able to fix the problem.

LGTM! I can't merge this though — it needs approval from both back-end and front-end folks 😊 Signaling that this is ready for final review by marking RTBC.

Let's first get @tedbow to review the changes to ComponentTreeHydrated(Test).

Looks good. I did my one suggestions

Back-end pieces are in place. Now needs front-end sign-off to land 🤓

Looks good. Thanks for the changes @Wim Leers

Reviewed FE only and approved as the FE person in the MR.

🥳 Thank you!

Rather than reopen this, Lauri said I should follow up in new issue so...

🐛 Adding a component with slots does not register the slots as children Fixed

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