The component preview should have a background: include theme's global asset libraries for component preview

A simple addition of

background-color: white;
color: black;

results in:

… but as you can see, that does not result in the correct styling, because it's supposed to look like this:

AFAICT the problem runs deeper: ✨ Preview component when selecting in left sidebar Fixed introduced this as just a <div> containing the markup.

✨ Preview component when selecting in left sidebar Fixed then enhanced this to use shadow DOM, for reasons stated by @jessebaker in #3466303-3: Follow-up for #3462636: component previews in left menu don't have style encapsulation → .

So … not sure how to fix this just yet 😬

Oh, but that component is the one provided by https://www.drupal.org/project/demo_design_system → — and there's at least one major problem with that component (see https://git.drupalcode.org/project/demo_design_system/-/tree/1.0.x/compo...): it does not contain a *.css file, but a *.scss file, which is not supported by SDC: https://www.drupal.org/docs/develop/theming-drupal/using-single-director... →

The same is true for https://git.drupalcode.org/project/demo_design_system/-/tree/1.0.x/stars....

It seems they're already aware of that: 📌 Create individual SDDS component css files from scss files Needs work 👍

IOW: I think we should still fix this issue, but there's a bigger problem at play for the specific example in the issue summary.

Regardless of 📌 Create individual SDDS component css files from scss files Needs work , we'd need to consider global libraries. Themes are allowed to specify these in *.info.yml via libraries:. We should load these libraries + dependencies of the component to ensure that the component preview renders accurately. +1 on ##5.

Ah, very good point!

That's very similar then to 🐛 Improve accuracy of styling in preview: attach default theme's asset libraries Closed: duplicate . But here, we'd need to update the default_markup returned by \Drupal\experience_builder\Controller\SdcController::getComponentsList() 👍

But upon hacking that in (i.e. assuming Olivero is the default theme), using:

diff --git a/src/Controller/SdcController.php b/src/Controller/SdcController.php
index 4d9f58f8..f01bacf0 100644
--- a/src/Controller/SdcController.php
+++ b/src/Controller/SdcController.php
@@ -137,8 +137,12 @@ final class SdcController extends ControllerBase {
       }
       $assets = AttachedAssets::createFromRenderArray([
         '#attached' => [
-          // @see \Drupal\Core\Plugin\Component::getLibraryName()
-          'library' => ['core/components.' . str_replace(':', '--', $component_plugin->getPluginId())],
+          'library' => [
+            // @todo do not hardcode this, instead get the actual default theme's global asset library; see https://git.drupalcode.org/project/experience_builder/-/merge_requests/152/diffs for how to do that
+            'olivero/global-styling',
+            // @see \Drupal\Core\Plugin\Component::getLibraryName()
+            'core/components.' . str_replace(':', '--', $component_plugin->getPluginId()),
+          ],
         ],
       ]);

… the right assets are loaded, but it still looks exactly the same.

Because:

I think this is related to shadow DOM isolation? 🤔 I bet I can figure out the root cause, but at this point, it'd be better investigated by somebody with more CSS and Shadow DOM knowledge. 😅

MR is created. The issue is not because of Shadow DOM, issue is in the heading component styling itself. I updated the component style. For the preview block, I remove the white text, and add a white background.

Thank you so much, @sea2709! 😄

The issue is not because of Shadow DOM, issue is in the heading component styling itself.

What was the precise root cause?

Why was I seeing CSS variables not getting their values resolved? Did you not see that too?

@Wim Leers
I noticed the heading component style was like that

[data-component-id="experience_builder:heading"] {
  &.h1 {
    letter-spacing: -0.01em;
    font-size: 1.75rem;
    line-height: var(--sp2);
  }

Based on the twig files, h1, h2, h3, etc should be tags not a class. And in case, we have heading tags, they should go first before the attributes in CSS styling.

I do see CSS variables not getting their values, let me check it!

I do see CSS variables not getting their values, let me check it!

Thanks! 🙏

Based on the twig files, h1, h2, h3, etc should be tags not a class. And in case, we have heading tags, they should go first before the attributes in CSS styling.

HAHAHA! 😅 That's quite the "oopsie" in 📌 Introduce an example set of representative SDC components; transition from "component list" to "component tree" Fixed 🙃

@Wim Leers:
I see that when we are using edit mode, we don't load the base libraries from the theme. Some of our components are using variables defined in the theme. There are 2 approaches acrossed my mind:

1. The base theme libraries should be loaded when previewing components. I tried this way, but ran into a problem when the CSS custom properties defined at the :root level, and they are not available in Shadow DOM, for example, in Olivero theme, they are

:root {
--sp: 18px;
...
}

This approach will work if we add :host, otherwise these variables cannot be applied in Shadow DOM.
I'm not sure if at the component level, when we use variables, we should define a default value or not!

2. The base theme libraries should be loaded when we are in XB editing mode, so all CSS custom properties defined on :root are available in shadow DOM, I updated my MR following this way.
So there are theme libraries loaded when we load XB

This is how I loaded the theme libraries

$base_page_build = $this->bareHtmlPageRenderer->renderBarePage([], '', 'page');
    $libaries = $base_page_build->getAttachments()['library'];
    $libaries[] = 'experience_builder/xb-ui';
    return (new HtmlResponse(self::HTML))->setAttachments([
      'library' => $libaries,
     ...

I don't have a strong opinion for this case, need some suggestions, and I'm willing to change my implementation for a better approach :-)

Played around with this. Used this branch, added examples text for heading, and added a .css file (red background and white text). I did have to bump the font size as it was tiny before I did that, but that's probably because I'm not pulling in the global css.

Nice to see something "real" :D Thanks!

@Kristen: The component preview is scaled down 50%, which makes many previews look so tiny. The preview wrapper is 600px width, and it is scaled down 50%, so the width is 300px.

@Wim Leers: Not sure if we already discussed about the preview size. I see we do some calculation for the scale ratio, but based on the code logic, it's always 50%

#15

I see that when we are using edit mode, we don't load the base libraries from the theme. Some of our components are using variables defined in the theme. There are 2 approaches across my mind:

Making sure: when you say "edit mode", do you mean the XB PoC UI at /xb/node/1?

If so: that should not load the default theme's asset libraries. That's the root document, the main frame. The default theme's global asset libraries should only be loaded in the two preview <iframe>s (one for "desktop", one for "mobile" — currently).

Since 📌 [needs design] [RESEARCH] Previewed component tree for Content Creator does not match the result for Visitor Active , those are being loaded.

That's why in #7, I modified the generated default_markup to include the Olivero (i.e. default theme) global asset libraries (a single one), which causes them to be loaded in the component preview's shadow DOM. That should be equivalent to loading them in the preview <iframe>s, should it not? 🤔 I have zero experience with shadow DOM, so I could be totally wrong! 😅
If that doesn't/can't work, then we'll need to start using <iframe>s for the component previews too. The use of shadow DOM was added in 🐛 Follow-up for #3462636: component previews in left menu don't have style encapsulation Fixed (as mentioned at the bottom of #2).

1. 🤔 So does that mean we'd need to modify the CSS rule from :root {…} to :root, :host {…}? Or … I guess … this would be a safe addition to make to core/themes/olivero/css/base/variables.pcss.css? Reference: https://developer.mozilla.org/en-US/docs/Web/CSS/:host
2. 👎 AFAICT that could too easily break the XB UI. The XB UI aims to be independent of default or admin theme.

If that doesn't/can't work, then we'll need to start using

s for the component previews too.

Great, thank you, @sea2709! 😊

@Wim Leers
I updated to use iframe component preview. This is the video I recorded to see how it works on my local
https://drive.google.com/file/d/1shf5KCvelJNG90R6VeCrO4KfGIQeof19/view

The preview popup shows up not really smoothly. Since it is an iframe, beside the component CSS file, it loads other theme libraries, so the rendering is kind of lagging. I put a script to resize the iframe height after it is loaded completely, originally, the iframe size is 1200px * 1600px, most of components are much shorter than this size, so I updated the height, not sure if it is needed.

I see that if we use iframe, we can get these things done:
1. Components can use CSS variables that are defined in the theme
2. Some components require executing scripts, like some sample components from Shoelace that we are using.

But the downside of it is to load many resource files then it takes some time to render.

I'm not concerned about the case when the iframe renders a mobile view instead of a desktop view since the preview container is small. We set the width for the iframe as 1200px, and then we scale it down, so there is no reason that the mobile media query takes effect.

I did a test on components using Shoelace library, I use both Shadow DOM and iframe, all the inputted HTML are the same for both cases.

This is the HTML structure inside the Shadow tree

And this is the HTML structure inside the iframe

I think the iframe is more reliable when some components need some scripts to render.

Working on failed jobs in the pipeline

@Wim Leers: When you have a chance, can you take a look at this MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/241 ?
This is how it works on my local https://drive.google.com/file/d/1hr2WXAg8V-xX2oC91i7oDHO78dYZHUO0/view?u...

This MR is using iframe and attach a src attribute to iframe for component preview. At first, I was about to use srcdoc for the iframe by building a preview HTML for each component, but then I saw this issue https://www.drupal.org/project/experience_builder/issues/3471070 🐛 [Performance regression] Loading the components takes >5s Active , I changed the implementation to build a component preview route.
I noticed that the MR's pipeline https://git.drupalcode.org/issue/experience_builder-3469856/-/pipelines/... is failed at the job cypress E2E, I searched through source code test cases we implemented, and found test cases that are checking CSS classes for ShadowDOM, since I'm no longer using ShadowDOM then the test cases fail. I'm not familiar with doing Cypress E2E test cases, but can give it a try if you think using iframe is a good approach.

Thanks :-)

Assigning do Lauri sees it

https://git.drupalcode.org/issue/experience_builder-3469856/-/pipelines/...

I just updated the Cypress E2E scripts for previewing components, it's working now, yeah!!! It took a while since I can't set up Cypress on my local!
I see the job UI eslint fails, I think it failed after I merged branch 0.x into my branch, so maybe there is some in progress UI eslint work on 0.x branch.

Assigning for @jessebaker to review this.

@sea2709 the work you have done in https://git.drupalcode.org/project/experience_builder/-/merge_requests/241 seems to be a good approach to me. As you mentioned it covers more use cases (like loading in the component's CSS and JS) than what we have now.

However I think more work is required before it can be merged. These issues do seem quite challenging to solve, but ultimately are required for a good UX.

1) On hover there is a flash/flicker where the tooltip container (the div with the dropshadow) shows but the iFrame has not yet loaded. Can this be improved to look smoother.

2) The scaling seems to be wrong. Ideally a small button would display full size but a large Hero component would be scaled to a maximum width to fit inside the tooltip

3) the height of the tooltip is fixed and a lot taller than it needs to be.
(see above screenshot)

@jessebaker: Thanks for your feedback :-) Will improve the UX based on your inputs!

I like how Gutenberg editor in Wordpress implements this feature https://wordpress.org/gutenberg/ and learning its implementation, still not figured out yet, but will dig into it more.

sea2709 → changed the visibility of the branch 3469856-heading-preview-component-style-not-correct to hidden.

sea2709 → changed the visibility of the branch 3469856-3 to hidden.

Per @tedbow, this is de facto blocked on #3471070. @tedbow is taking that on: #3471070-9: [Performance regression] Loading the components takes >5s → , to unblock/accelerate this issue 👍

sea2709 → changed the visibility of the branch 3469856-3 to active.

sea2709 → changed the visibility of the branch 3469856-3 to hidden.

sea2709 → changed the visibility of the branch 3469856-2 to hidden.

sea2709 → changed the visibility of the branch 0.x to hidden.

sea2709 → changed the visibility of the branch 3469856-3 to active.

sea2709 → changed the visibility of the branch 0.x to active.

@jessebaker / @Wim Leers :

The MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/3... is ready to review. The job Cypress E2E is failed at the test case "previews components on hover" https://git.drupalcode.org/issue/experience_builder-3469856/-/jobs/2737246, I need someone to support me fixing this test case since I cannot set up Cypress on my local, it's hard for me to make it right!

This is the video about how this feature works on my local https://drive.google.com/file/d/1dKujRCNeg4XvvoyXM6BItv8CKvh09o9V/view?u...

Sorry for taking it so long, I was kind of struggling on how to handle the iframe without much flickering and resizing it appropriately :-)

@sea2709 Please don't apologise, the work you have done here is fantastic and is going to make a huge impact to the UX and to our demo at DrupalCon.

I think I've resolved the Cypress issue and I've given some small feedback on your code but it's really great stuff!

@jessebaker: Thanks for resolving the Cypress issue and your suggestion changes, I applied them and it worked well :-) Learn new things today :-)

Whoa this is a really nice solution and the whole discussion above just seems high-quality and productive 👏.

In my manual tests I spotted a scaling edge case and something with the drupalicon component

(in the screenshots I brute-forced backgrounds on 0.x) so we can better see what was happening
In the test-only all props component, which you can get by enabling sdc_test_all_props, it scales much smaller. The w/h ratio is pretty different from any of the default components, so it makes sense that this wasn't spotted earlier.

The Drupalicon component preview has become an empty square

But on the other hand, this MR fixes the shoe icon. On 0.x it is just a wayward drop shadow, but looks great in the MR.

Assigned it back to me to check points that @bnjmnm mentioned!

@jessebaker, @bnjmnm
I made some changes in the code to calculate the size of the component wrapper in iframe to fix the issue when the drupalicon component is rendered as a square.
I tested the preview component feature with Starshot demo design system components, and this is how it looks
https://drive.google.com/file/d/13MIMsw0fxyf1fsXD21wEMMhCm0wtBzSn/view?u...

The slider component preview in my test is not quite right, not sure if it's the nature of this component, or should we need some treatments for it?

During the development, I found a bug related to Radix Tooltip where the tooltip content is rendered twice, this bug is reported at https://github.com/radix-ui/primitives/issues/3034

I move the status to Needs Review, even though Slider component preview isn't rendered well. Happy to have other thoughts? :-)

I have approved the front end code. Need approval from back end code owner.

I think there may still be a slight issue with the scaling of the preview on particularly tall components but that is a small enough, self contained issue that I don't think it needs to be addressed right now. I will raise a follow up.

Added some feedback and questions to the backend implementation.

Review context: 🌞☕️

It's one of the last sunny days in Belgium, with a nice and comfy 23ºC/74ºF outside. So I just moved outside, sitting in the shade, sipping my coffee with my girlfriend next to me.

Manual test

I almost spat out my coffee because of a deep, unavoidable "OOOOOHHHHHHHHHHHHHHHHHHHHHHHH!!!!!" 🤣

This is SO VERY NICE:

You can tell that I literally could not resist the temptation of hovering over multiple time 🤣

I think this is ready now. I'd like to give @longwave the chance to do a final pass, to ensure I addressed his review as he hoped we would 😊

Saving attribution.

This is ready to go from the backend side, assigning to @bnjmnm as the remaining reviewer.

So beautiful that Wim nearly spits out coffee… 😍☕😮

I think @jessebaker having approved it is sufficient; we don't need everyone to always sign off. Ben has plenty on his plate, so going ahead and merging!

🤩🚀

Thanks everyone for reviewing and making changes on my work 🤩🤩🤩 Such a relief when this one is shipped 😀

Absolutely fantastic work, @sea2709! 👏😄

wim leers → closed merge request !241

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