neos / neos-ui

Neos CMS UI written in ReactJS with Immutable data structures.
GNU General Public License v3.0
265 stars 135 forks source link

Empty content collections have no overlay in multi column elements #2871

Open dlubitz opened 3 years ago

dlubitz commented 3 years ago

Description

Not all empty collections of multi column elements are shown.

Steps to Reproduce

  1. Add a new node element with more than one content collection e.g: Neos.NodeTypes.ColumnLayouts:FourColumn

Expected behavior

All empty collections should have the overlay as placeholder.

image

Actual behavior

Just the first column (empty collection) has a placeholder. All other empty collection placeholder are not shown.

image

Seems that all following empty collections don't get an placeholder because their height become 20px, because of adding the first placeholder in the first column.

Affected Versions

Neos: 5.3.1

UI: 5.3.4

Sebobo commented 3 years ago

@dlubitz is this still the case? I remember this happening in the past, but can't reproduce right now in the Neos.Demo with UI 5.3.8

dlubitz commented 3 years ago

@Sebobo Yes, still there on live websites, Also in UI 7.0.3. Unfortunately I have no Demo running at the moment to verify if there is something different.

AFAIR this place is causing it: https://github.com/neos/neos-ui/blob/726b84894799ed72449e97b00da41dd201eb5deb/packages/neos-ui-guest-frame/src/dom.js#L159-L163

It checks for heightOfContentCollection < 20 which is not true anymore for all subsequent empty collections as the first one raise their heigt by geting a placeholder.

Sebobo commented 3 years ago

Hm not sure how to reproduce this reliably. Maybe depends also on custom CSS. Will try again later.

dlubitz commented 3 years ago

You're right. It's not an issue on the Demo Page.

Demo Page uses Neos.Demo:Content.Columns.Four NodeTypes, which have a different rendering as Neos.NodeTypes.ColumnLayouts:FourColumn.

Neos.Demo:Content.Columns.Four

<div class="row">
  <div class="col-sm-3">
    <div class="neos-contentcollection">
       <!-- EMPTY -->
    </div>
  </div>
  ...
</div>

Neos.NodeTypes.ColumnLayouts:FourColumn

<div class="container columns-3-3-3-3 neos-nodetypes-columnlayouts-fourcolumn">
    <div class="column neos-contentcollection">
       <!-- EMPTY -->
    </div>
  ...
</div>

In combination with flex it increases the height of all columns (and collections) to the same height of first content collection.

Example css

.columns-3-3-3-3 {
    display: flex;
    flex-wrap: wrap;
 }
.columns-3-3-3-3 .column {
    width: 25%;
    float: left;
}

Not sure if we can treat it as a bug or rather as an inconvenience based on implementation.

Sebobo commented 3 years ago

Yes now I got it too, thx.

Personally I dislike this 20px magic number. IMO I don't see why we shouldn't simply always add the overlay to empty collections. Especially because it allows custom global styling to help editors.

Maybe @skurfuerst or @grebaldi can shine some more light on why it was thought to be necessary to have this condition even if there are no child nodes?

freesh commented 2 years ago

I also stumbled across this behavior. There might be cases where you don't want an automatically generated overlay. E.g. if you want to render your own via js.

But I think this is more of an edge case and in most cases you want to show the editor where to add content.

The problem occurs, for example, when you have several collections in a flexbox with align-items: stretch;. As soon as one collection exceeds 19px in height due to childnodes or a custom styled overlay, all following collections can no longer get an overlay because they were stretched to the same height.

As an alternative, I would suggest rendering a data attribute with the value true in the fusion prototype Neos.Neos:ContentCollection, so that we can then overwrite it with false in our ContentComponent if we need no overlay. The ui then checks whether the data attribute is present or not and if it is missing, no overlay is rendered.

That puts the control in the integrator's hands and gives us a bit more flexibility und is less confusing i think . 😅

Maybe something like this:

Neos.Neos/Resources/Private/Fusion/Prototypes/ContentCollection.fusion

prototype(Neos.Neos:ContentCollection) < prototype(Neos.Fusion:Tag) {
  // ...
  attributes {
    // ...
    data-__neos-add-empty-collection-overlay = true
    data-__neos-add-empty-collection-overlay.@if.onlyRenderInBackend = ${node.context.inBackend && node.context.currentRenderingMode.edit}
    // ...
  }
}

Neos.Neos.Ui/packages/neos-ui-guest-frame/src/dom.js

export const createEmptyContentCollectionPlaceholderIfMissing = (collectionDomNode) => {
  if (collectionDomNode && collectionDomNode.hasAttribute('data-__neos-add-empty-collection-overlay')) {
    const hasChildNodes = Boolean(collectionDomNode.querySelector('[data-__neos-node-contextpath]'));
    const hasEmptyContentCollectionOverlay = Boolean(collectionDomNode.querySelector(`.${style.addEmptyContentCollectionOverlay}`));

    if (!hasChildNodes && !hasEmptyContentCollectionOverlay) {
      const emptyContentCollectionOverlay = document.createElement('div');
      emptyContentCollectionOverlay.setAttribute('class', style.addEmptyContentCollectionOverlay);
      collectionDomNode.appendChild(emptyContentCollectionOverlay);
    }
  }
};

Deactivation for a single collection:

// ...
    content1 = Neos.Neos:ContentCollection {
        nodePath = 'column1'
        attributes.data-__neos-add-empty-collection-overlay = false
    }
// ...

Tested with neos 7.3