- Issue created by @longwave
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
This MR builds on top of ๐ Preview entire page not just content area Active , so let's land that ASAP to make this MR (much) easier to review.
I did review the ADR already though!
- ๐ฌ๐งUnited Kingdom longwave UK
I've added the concept of a
region
nodeType in the layout tree sent from the backend and added some basic support for this to the frontend, hopefully the frontend team can start to work on this now, I don't foresee any major changes to the actual structure from here in order to complete the implementation of this, but there is still quite a way to go on both backend and frontend. - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Quoting the IS:
Let the frontend consider them as a single tree and set of components, with additions to identify regions.
๐ค
Is this intended to be temporary or permanent?
Most Content Creators will not be able to modify the "global regions" (so: component trees in the theme's
PageTemplate
config entity), typically only Site Builders will be allowed to do that.Or is that considered an afterthought for now, because we do not have the concept of "locking" a component (sub)tree yet, which we'll also need for ๐ฑ [later phase] [META] 7. Content type templates โ aka "default layouts" โ affects the tree+props data model Active ?
- ๐ฌ๐งUnited Kingdom longwave UK
Is this intended to be temporary or permanent?
Most Content Creators will not be able to modify the "global regions" (so: component trees in the theme's PageTemplate config entity), typically only Site Builders will be allowed to do that.
The API should only be sending things that are allowed to be modified. So for the Content Creator they would only get the content entity tree (whether the root of this is a single region, or just an array of components, is still to be determined), but for the Site Builder they would get the full PageTemplate and all regions, with the content entity embedded somewhere inside one of those regions. The frontend should not need to know about permissions or roles, it just allows the user to edit whatever is sent to it.
- ๐ฌ๐งUnited Kingdom longwave UK
One thing that I am still not clear on is how to delineate the point where the PageTemplate region ends and the content entity begins.
In Drupal the "content" region generally contains the "main content" block which is where the content entity is rendered. It is possible to place other blocks in the same region above or below the "main content" block.
I assume we want to continue this concept in XB? We already have the data model restriction that a PageTemplate must contain a "main content" block, so it feels like we should continue this in this data model. The "content" region should therefore contain a "block.system_main_block" component. Unlike other block components, we could force this specific component to have a single slot, which is where the content entity tree lives?
There is still a problem in that we need to limit the front end here further than we already have; the user needs to be prevented from moving components out of the "main content block" slot and into another part of the "content" region. But also, how do we explain the difference to the user of a component being inside the content entity/"main content block" and being in the "content" region but outside of the "main content block"?
We could try to sidestep this for now by only placing the "main content block" in the "content" region (perhaps along with the messages block), as this feels like a bit of an edge case; it's easy to add e.g. a "content top" region to a theme instead.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
#7: WFM! ๐
#8:
Hah, I just finished a whole write-up about that over at #3489302-28: Preview entire page not just content area โ , because @lauriii pointed out the preview was still imperfect. The reason for that is closely connected to what you just described here.
Details:
The "content" region should therefore contain a "block.system_main_block" component.
Let's start with that, but eventually, the main content block should be placeable in any region of the theme. But that depends on the UX for editing just the
PageTemplate
(independent of any concrete route) being defined and designed. For now your proposal is pragmatic and fine ๐There is still a problem in that we need to limit the front end here further than we already have; the user needs to be prevented from moving components out of the "main content block" slot and into another part of the "content" region.
Right!
That is exactly where we need the designs to provide clear affordances that make the user aware what they're editing: I think that by default the global regions should NOT be editable while editing e.g.
Page
orNode
7. Otherwise the Content Creator might unwittingly be modifying thePageTemplate
and hence affect all routes/HTML responses on the site, not just the one forPage
orNode
7.I also touched upon that need in #3489302-28: Preview entire page not just content area โ .
That last bit is actually why I'm concerned about this bit in the IS:
As well as being able to edit a node layout which lives inside the "main content" block, we also need to be able to place components in other theme regions in Experience Builder.
โฆ because that literally blurs the lines between "the component tree in the sole content entity being edited" and "the component trees in the theme's
PageTemplate
that literally affect the entire site". - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
That last blockquote in #9 is actually why I'm concerned about this bit in the IS:
As well as being able to edit a node layout which lives inside the "main content" block, we also need to be able to place components in other theme regions in Experience Builder.
โฆ because that literally blurs the lines between "the component tree in the sole content entity being edited" and "the component trees in the theme's
PageTemplate
that literally affect the entire site".Let the frontend consider them as a single tree and set of components, with additions to identify regions.
And this is AFAICT wrong? @lauriii himself said the same thing over at #3489302-26: Preview entire page not just content area โ , because the pieces he identified as missing in those screenshots aren't rendered by an XB component tree.
- ๐ซ๐ฎFinland lauriii Finland
The global regions should not be editable by default when editing a page or node. The interaction that design defined for this is that users need to double click a global region, which opens up the region in a "focus mode" where user is now allowed to make changes to the global region. How this looks visually has been defined here: https://www.figma.com/design/Ps3m4APGHIILsBrm0Uj31N/Experience-Builder?n....
It is possible to place other blocks in the same region above or below the "main content" block.
I'm not convinced we would need to support this. This is easy enough to workaround by providing a "Above content" and "Below content" regions so I'm not convinced that we should convolute the UX to add support for this use case.
Let's start with that, but eventually, the main content block should be placeable in any region of the theme.
If we need this, this seems like a setting for the theme. This doesn't seem something that would have to be editable in XB. I'm not aware of common use cases that would require moving the main content block to another region after the initial theme setup.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
This doesn't seem something that would have to be editable in XB
Oh wow, you're saying that in the XB-powered world, a theme would actually define in its metadata which region should contain the main content block (
Drupal\Core\Block\MainContentBlockPluginInterface
), and it should then contain nothing else? If so, we'll need to adjust the validation constraints for thePageTemplate
config entity.If so: do we want the same treatment for the page title (
Drupal\Core\Block\TitleBlockPluginInterface
) and messages (Drupal\Core\Block\MessagesBlockPluginInterface
)?That's crucial information not present in the that potentially would've made ๐ Introduce an XB `PageTemplate` config entity Active simplerโฆ ๐
- First commit to issue fork.
- ๐ฌ๐งUnited Kingdom longwave UK
@wim leers BlockPageVariant already has this fallback:
if (!$main_content_block_displayed) { $build['content']['system_main'] = $this->mainContent; } // If no block displays status messages, still render them. if (!$messages_block_displayed) { $build['content']['messages'] = [ '#weight' => -1000, '#type' => 'status_messages', '#include_fallback' => TRUE, ]; }
I think for a first iteration in XB it is fine to assume the same thing, ie. that the "content" region contains the messages block and main content block in that order.
Page title block should probably still be placeable by the user - and in fact there seems no hard requirement that it *must* be on a page, it's not critical?
- ๐ซ๐ฎFinland lauriii Finland
If so: do we want the same treatment for the page title (Drupal\Core\Block\TitleBlockPluginInterface) and messages (Drupal\Core\Block\MessagesBlockPluginInterface)?
99% of the time sites would display these foundational blocks in a region and there wouldn't be any complexity involved with it. However, there are some use cases where context is used so that the block could be displayed in a different region depending on the page.
The approach that I think would work here is to allow theme to decide a placement of a block. Once theme has decided on it's placement, it would not be available to be used in XB. This would particularly well for
Drupal\Core\Block\MessagesBlockPluginInterface
andDrupal\Core\Block\MainContentBlockPluginInterface
. Maybe it doesn't even have to be generic since it's quite specific to these two blocks.The outlier here is
Drupal\Core\Block\TitleBlockPluginInterface
because in most scenarios, it would not make sense to display the title block for Pages and Nodes because the title would be defined either in content or the content type template. However, other controllers may need the title to be displayed. If we implemented the generic way for themes to place blocks, we could potentially fit the title use case there too without introducing conditionality for the placement by allowing the template to not render the variable when the title is not desired. Otherwise I guess for now we'd just allow users to place the title in case that it's something they'd need.This is all really fascinating and brings me flashbacks from #2476947: Convert "title" page element into a block โ .
- ๐ฌ๐งUnited Kingdom jessebaker
Following a meeting between @wim leers, @longwave, @f.mazeikis and I, I created the following notes:
- At the top level, we will have an array of regions - always at least a `content` region which will replace โrootโ concept.
- We should default the tree to having a โcontentโ region on everything. Itโs special cased and will error if it doesnโt exist.
- Permissions handled back end - regions a user doesnโt have access to wonโt be sent (but the HTML in the preview will still be present).
- Suggestion: Always send full tree from client to server to enable editing more than one region in a single โround tripโ
- Validation: backend can return unique identifier - front end can use that to highlight the errored โthingโ where ever it might be
- Later: discuss with @Lauriii the repercussions/design behind what things do/donโt go into the temp store. I think the consensus was that pretty much everything should always save to temp and be published in one action at a later point to avoid things like a pattern existing but it contains components that were never published.
- ๐ฌ๐งUnited Kingdom f.mazeikis Brighton
wim leers โ credited f.mazeikis โ .
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
@longwave in #14: aha, so the
content
region becomes (very) special-cased! That can work. Note that thePageTemplate
config entity's validation constraints require those 3 special blocks to be present, so rather than doing that fallback logic inPageTemplateDisplayVariant::build()
(similar to the code you quoted fromBlockPageVariant::build()
), we can and IMHO should do it inPageTemplate::create()
. (There's precedent for that: see implementations ofEntityInterface::create()
).(Also, I forgot about the
content
region being assumed by Drupal core to exist, even though themes could not have such a region. It's reasonable, pragmatic, and not in anyway a BC break to assume that region, so: yay! ๐)@lauriii in #15:
- RE: main content + messages blocks. Sounds great to me! @longwave proposed both the main content & messages blocks in the
content
region. - RE: title block.
it would not make sense to display the title block for Pages and Nodes because the title would be defined either in content or the content type template.
For accessibility and consistency reasons (and others), it's important that the title (of a controller with some logic or of a content entity) is always rendered in the same place, using the same markup.
That's why I doubt the content entity's title can realistically become part of the content entity (template)'s component tree? ๐คLook at
/admin/structure/types/manage/article/display
in the Standard install profile: the article node's title field does NOT appear there! It also does not appear incore.entity_view_display.node.article.default.yml
at all. This is due to:->setDisplayOptions('view', [ 'label' => 'hidden', 'type' => 'string', 'weight' => -5, ])
not also doing
->setDisplayConfigurable('view', TRUE)
(see the code in
\Drupal\field_ui\Form\EntityDisplayFormBase::getFieldDefinitions()
: it filters away any field definition that is not "display-configurable").
@jessebaker in #16: Thank you so much for summarizing our conversation! ๐คฉ๐
RE: 1+2 โ yep, that's what @longwave explained in #14 and I now see can work.
RE: 4 โ this implies that a single request from the client might modify both the content entity currently visible in the XB UI and the active theme's
PageTemplate
config entity simultaneously! We discussed that in some detail, and @longwave believes this this is viable, although he agrees this is entirely new ground in all of Drupal (saving a content + config entity together in a single transaction), especially given that we'll have to save them into tempstore/a workspace and then publish it all together.
โ ๏ธ This does imply that thewse_config
contrib module will become a hard dependency for XB!
๐โโ๏ธ Alternatively we could've gone with the config overrides PoC I did back in April, but that was just for verifying we wouldn't need Workspaces to achieve the intended UX. I think since then, the design has evolved sufficiently, as well as technical discussions, to have concluded that XB will rely on Workspaces. I think it'd be good for @lauriii to formally document/state that though :)RE: 5 โ @tedbow and I got the basics of that "transform validation errors' server-side property paths to client-side request body equivalents" logic working in โจ AutoSave Entity Revision POC Active . So I'm convinced it's possible albeit not trivial. But both @longwave and @jessebaker believe it's important to keep the client A) as simple as possible, B) to prevent out-of-sync-sequential-updates to the preview: by being able to send everything at once from the client to the server (this issue:
PageTemplate
config entity + content entity, future:PageTemplate
config entity + content type template config entity aka ๐ฑ [later phase] [META] 7. Content type templates โ aka "default layouts" โ affects the tree+props data model Active + content entity), we minimize both round trips/latency and ensure a single update to the preview that immediately reflects changes across all those entities.๐ Point 5 above (the bolded bit at the end) is IMHO the strongest reason to go down this path of keeping the client blissfully unaware of any server-side representations, and make the server side much more complex by forcing it to map a single request body onto many entities to update. ๐
- RE: main content + messages blocks. Sounds great to me! @longwave proposed both the main content & messages blocks in the
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ Preview entire page not just content area Active is in, updating title.
Issue summary fully updated now.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Made @larowlan aware of this direction over at #3467954-31: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc. โ , to ensure he doesn't go in a conflicting direction.
- ๐ซ๐ฎFinland lauriii Finland
For accessibility and consistency reasons (and others), it's important that the title (of a controller with some logic or of a content entity) is always rendered in the same place, using the same markup.
That's why I doubt the content entity's title can realistically become part of the content entity (template)'s component tree? ๐คI don't think mandating this as a requirement matches well with real life requirements that sites have from design perspective. I agree that title has some nuance to it which makes it different from messages and the main.
Here are few example where it's easy to see there are many sites where there isn't a lot of consistency on how the title is displayed between pages:
discuss with @Lauriii the repercussions/design behind what things do/donโt go into the temp store. I think the consensus was that pretty much everything should always save to temp and be published in one action at a later point to avoid things like a pattern existing but it contains components that were never published.
Anything that would impact the live site should go through the temp store. I was originally thinking that maybe adding new sections wouldn't go through the temp store but there could be some interesting edge cases with that because someone might be creating a section using a component that only exists in the temp store. ๐ค
- ๐ฌ๐งUnited Kingdom longwave UK
Look at /admin/structure/types/manage/article/display in the Standard install profile: the article node's title field does NOT appear there!
This is only really due to historical reasons, core has been trying to unpick this for a long time and progress is slow: โจ [META] Expose Title and other base fields in Manage Display Active
- ๐ฌ๐งUnited Kingdom longwave UK
Agreed with @jessebaker that we should open child issues to handle some of the data model changes here instead of trying to do them all in one MR.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ Not all components have UUIDs, so rename to ID Active is in!
Let's rebase this :)
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
Another related issue. Going for a rebase here as ๐ Implement auto-save of the page template config entity Active will need the changes here
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
Pushed some work to start building the regions based on the page template. It's failing openapi spec validation at the moment
Will continue tomorrow.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
wim leers โ changed the visibility of the branch 3489899-add-global-regions to hidden.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
- ๐ Add setting to enable XB for page templates Active is in.
- As is ๐ Create a value object for the return of ComponentTreeHydrated::getValue Active , which @larowlan alluded to in his review.
- 4 E2E tests are still failing.
- I pushed one proposed commit (feel free to revert altogether) to outline how I think we should address @larowlan's review at https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...
=
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
Crediting Nathan who pointed me at cli viewport size as a possible cause of the failing e2e test when only in headless mode
- ๐ฌ๐งUnited Kingdom longwave UK
longwave โ changed the visibility of the branch 3489899-add-global-regions to active.
- ๐ฌ๐งUnited Kingdom longwave UK
Not sure we need
explicitInputName
both as a const and in the plugin metadata; we should pick one only. - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
@longwave in #33: to have it available in the annotation-based plugin definition' but also use that information from the plugin definition in the code. This is nothing new; I'm just matching the existing pattern that you and @f.mazeikis introduced (see
BlockComponent::SOURCE_PLUGIN_ID
) in ๐ Add support for Blocks as Components Active ? ๐ -
wim leers โ
committed 59f4c9bc on 0.x authored by
longwave โ
Issue #3489899 by longwave, larowlan, wim leers, jessebaker, lauriii,...
-
wim leers โ
committed 59f4c9bc on 0.x authored by
longwave โ
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ฅณ
See y'all in:
- ๐ Some components cannot be used in XB because UI prevents SDC props being named `name` Active
- ๐ Implement auto-save of the page template config entity Active
- ๐ Allow source components to control how hydrated values are mapped into the clientside model Active
- ๐ [PP-1] Add contextual menu frontend support for moving components into regions Postponed
- And last but not least: @longwave is going to file a follow-up for https://git.drupalcode.org/project/experience_builder/-/merge_requests/4... โ we hopped on a Zoom to clarify that :)
(@longwave please link that issue and mark this as when you've done that!)
Post-humously bumped to considering how many issues this unblocked! ๐
- ๐ฌ๐งUnited Kingdom longwave UK
Opened ๐ Rename component source plugin configuration keys for clarity Active for #38.5.