Render component tree inside the default theme, as a regular page

Created on 14 August 2024, about 1 month ago
Updated 9 September 2024, 10 days ago

Overview

Context/evolution:

  1. 📌 Connect client & server, with zero changes to client (UI): rough working endpoints that mimic the UI's mocks Needs review introduced the /api/preview route. And was naïve.
  2. 📌 Load assets inside preview Active updated it to load component assets into the preview <iframe>.
  3. 📌 Refactor SdcController::preview() to use ComponentTreeHydrated, and update it to support nested components Fixed started using the same component tree rendering as the XB field formatter, guaranteeing the markup is identical.
  4. 🐛 Improve accuracy of styling in preview: attach default theme's asset libraries Closed: duplicate 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:

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:

📌 Task
Status

Fixed

Component

Page builder

Created by

🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

Merge Requests

Comments & Activities

  • Issue created by @Wim Leers
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
  • 🇫🇮Finland lauriii Finland

    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.

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
  • Assigned to tedbow
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
  • Assigned to longwave
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    @longwave has started pushing this forward 👍

  • First commit to issue fork.
  • Status changed to Needs review about 1 month ago
  • 🇬🇧United Kingdom longwave UK

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

  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    @longwave's self-review made me realize this is unexpectedly connected to 📌 Remove MSW infrastucture Active , but both can happen independently 👍

  • Assigned to lauriii
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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.

  • 🇬🇧United Kingdom longwave UK

    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.

  • Issue was unassigned.
  • 🇫🇮Finland lauriii Finland

    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.

  • Assigned to lauriii
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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.

  • 🇬🇧United Kingdom longwave UK

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

  • Merge request !185Draft: Resolve #3468106 "Render all blocks" → (Open) created by longwave
  • 🇬🇧United Kingdom longwave UK

    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.

  • Assigned to jessebaker
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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

  • 🇬🇧United Kingdom jessebaker

    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.

  • 🇬🇧United Kingdom longwave UK

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

  • 🇫🇮Finland lauriii Finland

    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.

  • Assigned to Wim Leers
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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.

  • Issue was unassigned.
  • Status changed to RTBC 28 days ago
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
  • Pipeline finished with Skipped
    28 days ago
    #261293
  • Status changed to Downport 28 days ago
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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

    They agreed.

    So: 🚢

  • Status changed to Fixed 10 days ago
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    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 .

Production build 0.71.5 2024