PageLayoutSource is sometimes not rendered

started

Done.

The phpstan fails are probably not related to this work.

A rebase should fix ci.

Indeed

Thanks for the review.

Review

There is no indication on what it is, mostly the fact that slots are not real regions in Drupal sense

The source plugin label is "Page layout (from active theme)", it may not be the best label, but I don't have better idea right now. Do you?

you can not configure it, or adapt the layout

That's the point, this source plugin is the exact representation of the rigid, hardcoded, not configurable, page.html.twig.

moving a block to an other region change only the order

Oh! Really? That's a bug, i will a have a look.

you can apply styles or visibility on it, which then make it like a container, but it is not and styles will apply to a child it seems...

Styles: As far as I know, page.html.twig doesn't have attributes, but UI styles is supposed to manage this.
Visibility: Weird.
I will have a look.

UX improvements

Page Layout source was created as a way to merge existing block layout configuration so user do not have an empty page. But then we create a unexplained component with regions without real markup.
And problem is worst when using a proper theme with layout components, you basically on every new Page Layout have to remove the page layout to build your page.

I agree there is something to consider here. The automatic initialization with this PageLayoutSource plugin is relevant for the projects keeping Block Layout as a fallback/default. But not for the projects already using a Page Layout entity without any condition plugins set as a fallback/default.

Today, this situations is managed by:

• always importing PageLayoutSource plugin when creating a PageLayout entity from scratch
• but also proposing a "Duplicate page layout feature" so users can start from the default Page Layout entity if they want

But we can do better.

Start from a flat imported list of blocks from blocks layout?
Start with the minimal core block layout without looking on existing block layout state? with title, messages, help, tags, content

I am afraid those proposals are not fixing the fallback/default pages dichotomy because:

• an user not using Block Layout anymore will not be interested by a flat imported list of blocks from blocks layout
• an user using a Page Layout as fallback/default will prefer to use it as a starting point instead of a minimal hardcoded list

To improve the UX, we can make this choice more obvious by exposing it directly in the creation page (and not the edit page) of the config entity type:

• by adding a select input with Block Layout and all page layout entities without any condition plugin set
• if there is no page layout entities without any condition plugin set, we keep the select as disabled/readonly

Screenshot:

What do you think?

Discussed with Jean.

Review:

• moving a block to an other region change only the order > ✅ Already fixed by the MR
• you can apply styles or visibility on it, which then make it like a container, but it is not and styles will apply to a child it seems... > ✅ Styles already fixed by the MR ⚠️ visibility to check again

UX improvements:

• OK for select list at create but with minimal/ block layout / blank instead of looking for page layout entities without any condition plugin set.
• Pierre will try to do the change in the current MR. But if too big/risky, it will be a follow-up feature request.

Review

Styles are using the "drilling" system of UI Styles which is sometimes working weird because the Drupal Render API is weird.
For example, with UI suite DaisyUI, it drills until the navbar slots.

So, to have a consistent behavior, I have wrapped the renderable in a html_tag which will host the styles and other dynamic attributes.

For visibility, I don't see surprising behaviour in Preview and final result. But I see weird stuff in Builder/Canvas: the renderable is not rendered anymore, that's normal, but the children are. Not being familiar with Visibility panel, i don't know if it is OK, but it doesn't seem something introduced by the current work which is fixing already a lot.

UX improvments

The create form page will be discussed after beta6.

Oh shout! you are right.

The current state of the MR is not working with every theme:

• UI suite DaisyUI: ✅ good layout rendering and styles correctly applied
• UI suite Material: ✅ good layout rendering and styles correctly applied
• UI suite DSFR: ✅ good layout rendering and styles correctly applied
• UI suite Bootstrap: ❌ I reproduce the error
• UI suite USWDS: ❌same error
• bootstrap with I suite Bootstrap styles: ✅ good layout rendering and styles correctly applied

I will do a change, test with all themes I have available, so i will send to review only when i am 100% sure.

Discussed with @mogtofu33 at weekly meeting.

We will update the entity create form page, but instead of proposing:

1. Import from Block Layout
2. Default page layouts (A default page layout is a layout with no condition plugin attached)

We will propose:

1. A flat set of default blocks (title, message, main...) and/or a completely empty option
2. Import from Block Layout

Hi Jean, your help and wording proposals are welcomed 🤗

I would just advise to not introduce the word "shell" which sounds technical and is not used anywhere in Display Builder, UI Suite or Drupal.

Let's have this without the wrapper in the code to have a follow up issue, in every case it is already better than last beta.

Sure. I will create the follow up issue.

However, I am afraid the merged commit is not working properly (especially for BuilderPanel), because this part has been removed:

return [
  // `page` renderable doesn't accept #attributes, so let's wrap it up to
  // be able to apply styles and other dynamic attributes.
  '#type' => 'html_tag',
  '#tag' => 'div',
  'content' => $page,
];

But not the related content key here:

$build['content'][$slot_id] = $slot;

So, we also need to fix this ASAP.

Folllow-up: #3614776: Make PageLayoutSource compatible with attributes