Add support for global regions

Created on 26 November 2024, 23 days ago

Overview

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.

Proposed resolution

Extend the layout tree and model storage to cover the entire page including all regions.
Handle the separation between the two in the backend.
Let the frontend consider them as a single tree and set of components, with additions to identify regions.

User interface changes

๐Ÿ“Œ Task
Status

Active

Version

0.0

Component

Page builder

Created by

๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @longwave
  • Merge request !429#3489899 "Add global regions" โ†’ (Merged) created by longwave
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK
  • ๐Ÿ‡ง๐Ÿ‡ช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 or Node 7. Otherwise the Content Creator might unwittingly be modifying the PageTemplate and hence affect all routes/HTML responses on the site, not just the one for Page or Node 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 the PageTemplate 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 and Drupal\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:

    1. At the top level, we will have an array of regions - always at least a `content` region which will replace โ€˜rootโ€™ concept.
    2. We should default the tree to having a โ€˜contentโ€™ region on everything. Itโ€™s special cased and will error if it doesnโ€™t exist.
    3. 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).
    4. Suggestion: Always send full tree from client to server to enable editing more than one region in a single โ€˜round tripโ€™
    5. Validation: backend can return unique identifier - front end can use that to highlight the errored โ€œthingโ€ where ever it might be
    6. 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
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    @longwave in #14: aha, so the content region becomes (very) special-cased! That can work. Note that the PageTemplate config entity's validation constraints require those 3 special blocks to be present, so rather than doing that fallback logic in PageTemplateDisplayVariant::build() (similar to the code you quoted from BlockPageVariant::build()), we can and IMHO should do it in PageTemplate::create(). (There's precedent for that: see implementations of EntityInterface::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 in core.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 the wse_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. ๐Ÿ‘

  • ๐Ÿ‡ง๐Ÿ‡ช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:

    1. https://www.doritos.com/
    2. https://www.jaffa.fi/
    3. https://www.acquia.com/

    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 ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
    1. ๐Ÿ“Œ Add setting to enable XB for page templates Active is in.
    2. As is ๐Ÿ“Œ Create a value object for the return of ComponentTreeHydrated::getValue Active , which @larowlan alluded to in his review.
    3. 4 E2E tests are still failing.
    4. 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...
    5. =

  • ๐Ÿ‡ฆ๐Ÿ‡บ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

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Crosspost

  • ๐Ÿ‡ฌ๐Ÿ‡ง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 ? ๐Ÿ˜…

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Pipeline finished with Skipped
    about 15 hours ago
    #372796
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    ๐Ÿฅณ

    See y'all in:

    1. ๐Ÿ› Some components cannot be used in XB because UI prevents SDC props being named `name` Active
    2. ๐Ÿ“Œ Implement auto-save of the page template config entity Active
    3. ๐Ÿ“Œ Allow source components to control how hydrated values are mapped into the clientside model Active
    4. ๐Ÿ“Œ [PP-1] Add contextual menu frontend support for moving components into regions Postponed
    5. 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
Production build 0.71.5 2024