Improve accuracy of styling in preview: attach default theme's asset libraries

Where is this metropolis coming from? I can find base.css at

ui/node_modules/@radix-ui/themes/src/styles/tokens/base.css

… but I cannot find metropolis anywhere in there.

Aha, it's core/themes/olivero/css/base/fonts.pcss.css that does this. This is part of the olivero/global-styling asset library. Which is declared as the sole global asset library in oliver.info.yml 👍

👇

That asset library must be attached in \Drupal\experience_builder\Controller\SdcController::preview().

(That'll also allow reverting most of 🐛 First `preview` request fails to generate a response for empty layout Needs review and will hence simplify that controller 👍)

This is a back-end issue, not a front-end one!

Attached the default theme CSS assets. One test is failing :(

Great start! 👍😄

Fortunately, I think this can be even simpler 😊 See review on the MR!

Addressed feedback in MR.

One test failed. The failure looks legit:

     AssertionError: expected null to exist
      at Context.eval (webpack://vite-template-redux/../tests/src/Cypress/cypress/e2e/xb-general.cy.js:60:14)

… if we look at line 60, it's this bit:

    // Confirm that the iframe loads the SDC CSS.
    cy.getIframe()
      .its('head').should('not.be.undefined')
      .then((head) => {
        expect(head.querySelector('link[rel="stylesheet"][href*="experience_builder/components/my-hero/my-hero.css"]'))
          .to.exist
      })

You can see that output here: https://git.drupalcode.org/issue/experience_builder-3466571/-/jobs/23766...(failed).png

👇

Either this is detecting a genuine bug, or the test assertion needs to be updated.

Actually … it works fine locally? 🤔 Re-running the Cypress E2E CI job.

I wonder if a more recent upstream commit changed it. I'll push a change.

Applied my own suggestion to address my review feedback, which also triggers a build against 0.x HEAD, and hopefully now it'll pass 🤞

Recent changes work fine. But getting couple of warnings.

Warning: Undefined array key "#attached" in Drupal\experience_builder\Controller\SdcController->preview() (line 356 of modules/contrib/experience_builder/src/Controller/SdcController.php).

Warning: Trying to access array offset on null in Drupal\experience_builder\Controller\SdcController->preview() (line 356 of modules/contrib/experience_builder/src/Controller/SdcController.php).

I made it pass tests, but:

1. one bug is still present, see MR review
2. it's not using the correct infrastructure yet, see MR review
3. we should expand the Cypress E2E test coverage

Good job getting it to this point already, @omkar-pd! 👏 And thank you! 😊🙏

Thanks @Wim Leers.

Upon checking the network tab, I see

The website encountered an unexpected error. Try again later.

Error: Only arrays and Traversables can be unpacked in Drupal\experience_builder\Controller\SdcController->preview() (line 356 of modules/contrib/experience_builder/src/Controller/SdcController.php).

@Wim Leers.

I've addressed the given feedback.
Had to add couple of extra lines to fix errors and warnings. Please review.

Error: Only arrays and Traversables can be unpacked in Drupal\experience_builder\Controller\SdcController->preview() (line 356 of modules/contrib/experience_builder/src/Controller/SdcController.php).

Warning: Undefined array key "#attached" in Drupal\experience_builder\Controller\SdcController->preview() (line 356 of modules/contrib/experience_builder/src/Controller/SdcController.php).

Warning: Trying to access array offset on null in Drupal\experience_builder\Controller\SdcController->preview() (line 356 of modules/contrib/experience_builder/src/Controller/SdcController.php).

Still needs tests.

Last step: tests.

All this needs is one extra assertion for an additional CSS file in:

    // Confirm that the iframe loads the SDC CSS.
    cy.getIframe()
      .its('head').should('not.be.undefined')
      .then((head) => {
        expect(head.querySelector('link[rel="stylesheet"][href*="experience_builder/components/my-hero/my-hero.css"]'))
          .to.exist
      })

🙏

Added an assertion for the olivero css file (Don't have much experience writing tests 😊).

LGTM!

Cypress E2E test is failing though … re-testing to see if that's an ephemeral failure.

Also, I'd like @jessebaker to do the final sign-off on this, given he reported it.

Re-testing once more… reliability of the Cypress E2E CI job is being improved in 📌 Document Cypress test best practices (especially how to write reliable tests) Needs review . 👍

I have merged in 0.x since the MR to harden the Cypress tests was merged in but this is still failing on a test so that needs to be addressed before I can approve/merge this.

      cy:command ✔  its	.head
      cy:command ✔  assert	expected **<head>** not to be undefined
      cy:command ✔  assert	expected **undefined** to exist in the DOM
      cy:command ✘  assert	expected **null** to exist

That's indeed the updated test coverage 👆, so something must be wrong in here:

    cy.getIframe()
      .its('head').should('not.be.undefined')
      .then((head) => {
        expect(head.querySelector('link[rel="stylesheet"][href*="experience_builder/components/my-hero/my-hero.css"]'))
          .to.exist;
        expect(head.querySelector('link[rel="stylesheet"][href*="/core/themes/olivero/css/base/base.css"]'))
          .to.exist;
      })

@Wim Leers

I see the same test failing for a super simple MR.
https://www.drupal.org/project/experience_builder/issues/3467818 📌 Update XB to match the latest Drupal 11 phpcs.xml.dist- 2024-08-13 Needs review

I Improved the test result output for this to state what is being looked for in <head> and, on failure, what is actually in <head> and it doesn't look like the base.css is in there.

 AssertionError: Tried to find [href*="/core/themes/olivero/css/base/base.css"] in <head> <link rel="stylesheet" media="all" href="/web/core/../modules/custom/experience_builder/components/my-hero/my-hero.css?si5uut">
<style>.preview-dragging .sortable-list{
  min-height: 2rem;
}
.preview-dragging .sortable-list:empty{
  background: #fff;
}
.slot-container {
  display: flex;
  > .sortable-item {
    width: 100%;
  }
}
.sortable-list:empty {
  min-height: 1.5rem;
  border: 2px dashed #ccc;
  position: relative;
}
.sortable-list:empty:after {
  content: 'Slot';
  top: 0;
  right: 0;
  text-align: right;
  position: absolute;
}
.sortable-item {
  cursor: grab;
}
.preview-dragging *,
.preview-dragging {
  cursor: grabbing;
}
.sortable-ghost {
  opacity: 0.5;
  padding: 0;
  height: 2rem;
  max-height: 2rem;
  width: 100%;
  margin: 0;
  clear: none;
  position: relative;
  background: linear-gradient(135deg, #DDD 12.50%, transparent 12.50%, transparent 50%, #DDD 50%, #DDD 62.50%, transparent 62.50%, transparent 100%) center / 5.66px 5.66px;
  outline: 1px solid #ccc;
  box-shadow: none;
  flex-grow: 1;
  overflow: hidden;
}
.sortable-ghost * {
  visibility: hidden;
}
</style>
: expected null to exist

Fixed tests by installing and setting Olivero as the default theme in XBTestSetup.php.
Back to Needs Review.

There's a conflict with 🐛 Account for GitlabCI intermittent path differences in SDC CSS test Fixed — should be easy to resolve :)

Resolved Conflicts.

Not yet passing tests. Re-running.

I showed in #18 that this is not yet quite yielding the kind of preview we'd like.

Yet, this issue still is a step in the right direction, so it should land 👍

To make the preview actually good enough, we need design and research. Created 📌 [needs design] [RESEARCH] Previewed component tree for Content Creator does not match the result for Visitor Active for that.

Checking the failed pipelines.

Timed out retrying after 4000ms: Expected to find element: button[aria-label="Add section"], but never found it.

If I run the same tests with Claro as a default theme then they pass without error but error occurs on Olivero. 😕

Due to the scope clarification by @lauriii over at #3468106-4: Render component tree inside the default theme, as a regular page → , this issue has become pointless to finish 🙈 Sorry. I'll credit y'all though!

👍👍

Wim Leers → closed merge request !152