Overview

Context/evolution:

  1. #3455898: Connect client & server, with zero changes to client (UI): rough working endpoints that mimic the UI's mocks introduced the /api/preview route. And was naïve.
  2. #3462450: Load assets inside preview <iframe> when components are added updated it to load component assets into the preview <iframe>.
  3. #3463986: Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components started using the same component tree rendering as the XB field formatter, guaranteeing the markup is identical.
  4. #3466571: Improve accuracy of styling in preview: attach default theme's asset libraries updated it to also load the default theme's global CSS/JS assets into the preview <iframe>.

That last step results in the following improvement:

Reference: what does the first article look like in the default theme?
Before
After

Source: #3466571-18: Improve accuracy of styling in preview: attach default theme's asset libraries.

👆🐛 That still does not look sufficiently similar, because the default theme (Olivero) has certain markup present, and the global CSS/JS assets apply to those.

Proposed resolution

TBD, because:

  • The design glosses over this challenge.
  • Assuming we'd want to reuse the default theme's html.html.twig and page.html.twig (which would be necessary for Olivero, and likely for most front-end themes)

User interface changes

#10: Olivero footer and background show up in the XB preview:

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Keeping at normal priority, so @lauriii can decide whether this is critical for #3454094: Milestone 0.1.0: Experience Builder Demo or not.

wim leers’s picture

Issue summary: View changes
lauriii’s picture

Title: [needs design] [RESEARCH] Previewed component tree for Content Creator does not match the result for Visitor » Previewed component tree for Content Creator does not match the result for Visitor
Issue tags: -Needs design, -Needs documentation

The designs are already showing that the UX should be similar to Layout Builder so that the page is rendered with the blocks in place to show how the content looks in context of the page. This has some positive effects on the UX but some negative impacts as well because it does mean that we are displaying more content in the current space. We may need to consider implementing a more complex renderer that simplifies the preview and provides a "focus mode" where only the content that is currently being edited is displayed.

wim leers’s picture

Title: Previewed component tree for Content Creator does not match the result for Visitor » Render component tree inside the default theme, as a regular page
Priority: Normal » Critical
Parent issue: » #3450586: [META] Back-end Kanban issue tracker
wim leers’s picture

Assigned: Unassigned » tedbow
wim leers’s picture

Assigned: tedbow » longwave

@longwave has started pushing this forward 👍

longwave made their first commit to this issue’s fork.

longwave’s picture

Status: Active » Needs review
StatusFileSize
new51.91 KB

First attempt at this, the Olivero footer and background show up in the XB preview.

wim leers’s picture

Issue summary: View changes
wim leers’s picture

@longwave's self-review made me realize this is unexpectedly connected to #3467674: Remove MSW infrastucture + `pages` CI job, but both can happen independently 👍

wim leers’s picture

Assigned: longwave » lauriii
Issue tags: +Needs product manager review

We now need feedback from @lauriii, to assess whether this is sufficient, or whether he expects e.g. the header/main menu etc to also show up.

longwave’s picture

Assigned: lauriii » longwave

Happy to change this so it renders more of the page (e.g. header/footer blocks as well) but it's not clear whether that is required or if it would get in the way in some cases.

longwave’s picture

Assigned: longwave » lauriii
lauriii’s picture

Assigned: lauriii » Unassigned
Issue summary: View changes
Issue tags: -Needs product manager review
StatusFileSize
new138.48 KB

Nice! I think we may want to render the header and footer blocks too. How much work it would be to build that so that me and the designers could test that? FWIW, I don't think we need to block this issue on that – this could be done in a follow-up.

Something I noticed was that bunch of stuff is not rendering outside the frame which is pretty confusing. I think that's something that @jessebaker or someone else from the frontend team should investigate.

wim leers’s picture

Assigned: Unassigned » lauriii

Something I noticed was that bunch of stuff is not rendering outside the frame which is pretty confusing. I think that's something that @jessebaker or someone else from the frontend team should investigate.

I bet that part is caused by https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

Nice! I think we may want to render the header and footer blocks too.

I interpret this as: Olivero's page.html.twig's regions:

  • header
  • primary_menu
  • secondary_menu
  • hero
  • highlighted
  • breadcrumb
  • social
  • content_above
  • content
  • sidebar
  • content_below
  • footer_top
  • footer_bottom

… or did you also want the hero etc? Either way, I think the consequence is quite clear: it'll depend on the front-end theme which regions we want to render the blocks for inside the XB preview. 👉 XB will need third-party settings to track this in the THEME.settings config for the default front-end theme.

The implementation for rendering the entire page but with only a subset of blocks means XB should have something like \Drupal\block\Plugin\DisplayVariant\BlockPageVariant and \Drupal\block\EventSubscriber\BlockPageDisplayVariantSubscriber but make it affect only the XB preview route.

longwave’s picture

Issue summary: View changes
StatusFileSize
new68.99 KB

So if we render the full page instead of the bare page, we get the header and footer but also the toolbar...

longwave’s picture

MR!185 renders all blocks as suggested in #16 but as per #17 I think we need to restrict this - we don't want the toolbar showing up, and we probably want to remove other blocks too. This could be a region setting, but it could also be a block condition? It seems likely there are some blocks people will want to show in XB and others that they don't, and perhaps hardcoding it by region in the theme isn't enough?

To me MR!184 should be merged here to unblock progress and then the decisions about additional blocks deferred to a followup.

wim leers’s picture

Assigned: lauriii » jessebaker

So if we render the full page instead of the bare page, we get the header and footer but also the toolbar...

We can mitigate that by implementing hook_page_top(), using hook_module_implements_alter() to move XB's implementation last, and then removing everything if the current route is the preview route.

but it could also be a block condition?

I forgot this for a moment! And block conditions might use routes. And so then the preview's set of blocks might be different (or just behave/look different) than the actual end result 😬

Massive can of worms.

To me MR!184 should be merged here to unblock progress and then the decisions about additional blocks deferred to a followup.

+1

jessebaker’s picture

StatusFileSize
new94.82 KB

Cool, the preview is now looking a lot more like the front end!

RE @lauriii in #16

Something I noticed was that bunch of stuff is not rendering outside the frame which is pretty confusing. I think that's something that @jessebaker or someone else from the frontend team should investigate.

I think the broken layout you are seeing is "correct" - in so much as it now matches the styles seen on the front end. Before it was not an accurate representation of the actual page as it is displayed.

If you visit /node/1 you will see all the content is shoved over to the right just as it is in the XB. On a quick investigation it seems that now we are correctly loading in the CSS there is a css rule to set <img> tags to have display: block; and the image we load in the default component has height and width attributes set to 1024.

longwave’s picture

Reran stylelint, seems you have to run it from /core otherwise it only picks up some of the config?

lauriii’s picture

Thanks for opening a MR for rendering the full page! I'm pretty sure that's the approach we'll go eventually. There's many problems in that MR which I know we're aware of. Maybe we could merge the bare page preview first and open a follow-up to move to using the full page renderer? This would allow us to move faster because AFAIK the full page renderer is not on the critical path for the first milestones.

… or did you also want the hero etc? Either way, I think the consequence is quite clear: it'll depend on the front-end theme which regions we want to render the blocks for inside the XB preview. 👉 XB will need third-party settings to track this in the THEME.settings config for the default front-end theme.

I would expect it to render the preview just as is when rendering the actual node page. What I wouldn't think we should render there are our "admin UI" elements in the frontend: i.e. admin toolbar and contextual links.

wim leers’s picture

Assigned: jessebaker » wim leers
wim leers’s picture

This causes a visual regression when the "hero" image is fairly wide:


👆 This appears to be a CSS problem, AFAICT because the user agent stylesheet of

img {
    overflow-clip-margin: content-box;
    overflow: clip;
}

is overridden by Olivero to

img, video {
    display: block;
    max-width: 100%;
    height: auto;
}

That deserves its own follow-up.

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community

  • Wim Leers committed f6f23b8d on 0.x authored by longwave
    Issue #3468106 by longwave, Wim Leers, lauriii, jessebaker: Render...
wim leers’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Discussed with @jessebaker + @lauriii whether to merge as-is and defer fixing #26 in a follow-up.

They agreed.

So: 🚢

wim leers’s picture

Status: Patch (to be ported) » Fixed
Issue tags: -Needs followup
Related issues: +#3220566: Olivero: wide image can overlap sidebar when toolbar open and in vertical mode

A core issue for #26 already exists: #3220566 — just commented on it: #3220566-25: Olivero: wide image can overlap sidebar when toolbar open and in vertical mode.

Status: Fixed » Closed (fixed)

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