🇬🇧United Kingdom @jessebaker

Merged!

jessebaker → made their first commit to this issue’s fork.

No longer postponed/blocked.

Do you think this should be a beta blocker rather than a stable blocker?

Obviously the ramifications are not so serious as data integrity or even severe in terms of user impact - however, we have put a lot of time and energy into creating a 'native feeling' to our editing experience and reducing flicker and FOUC has been a large part of that. The impact is on first impressions and the confidence brought from a feeling of robustness.

Here is a gif showing the current state. For clarity it's only an issue when there is an empty slot on the page. This is an SDC. The effect is the same with a code component. Each time a prop is updated and the preview updates the content below the slot appears to jump up and then pop back down again as the page renders without the space and then the space is inserted.

For now, I'm going to remove stable and beta blocker labels and treat this as a "nice to have" because in light of conversations around other priorities this not critical and can be fixed after beta.

I also think that perhaps this CAN be addressed client side if we tackle it in a different way given that it's non-trivial to handle on the server. It will need some thought but there are options to explore.

Reviewed and tested too. Merged!

jessebaker → made their first commit to this issue’s fork.

MR !1024 is now ready for review.

MR !990 is now over at 📌 Refactor sortable props form in code component editor Active

This is nice! Merged.

jessebaker → made their first commit to this issue’s fork.

jessebaker → created an issue.

🎉

This has been addressed in ✨ Refactor Layers UI to use DnDKit Active !

ui/src/features/layout/previewOverlay/snapRightToCursor.ts
https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

jessebaker → made their first commit to this issue’s fork.

It looks like the move away from SortableJS has magically solved this one for us!

@johnwebdev your MR is great! It fixes the issue as far as I can tell. I've added to it to fix another issue that's been bothering me!

The way the scaling is applied meant that when a component is scaled down, the rounded corners of the Panel were ALSO scaled down which meant the bigger a component, the smaller the rounded corner radius. You can see this in the screenshots in #5.

I also took a moment to tidy up the code and remove some bits that weren't needed.

jessebaker → made their first commit to this issue’s fork.

hooroomoo → credited jessebaker → .

From your video @mayur-sose it looks like you are slightly misunderstanding which behaviour needs testing.

The expected behaviour is as follows:
When dragging a component the is already on the layout the component that you start to drag should fade out to indicate which one you have "picked up". The fade out is based on which component started the drag operation, not which component your mouse is over as you are dragging.

Prior to this fix, the behaviour above only happened for SDC components, not Code Components. It should now happen no matter the component source.

By contrast, in your video you are dragging a new component from the library and then moving the mouse over a component in the preview. There is no expectation that the component in the preview will fade out in that scenario so this is not a regression.

When does XB render an empty region? Only when it's just had its last component instance removed?

This is a good point - I am specifically talking about the Content "region" when a new page is loaded. Other global regions don't need this.

It currently looks like this:

<!-- xb-region-start-content -->
<!-- xb-region-end-content -->

and I'd like to see this instead:

<!-- xb-region-start-content -->
<div class="xb--region-empty-placeholder"></div>
<!-- xb-region-end-content -->

For empty slots [...]

1. Ah, yes. I agree this would be blocked on that. I didn't realise the HTML comments were not already limited to only the XB preview
2. Code components: We are already inserting the HTML comments around slots for code components. I think determining if they are empty is tricky. Perhaps inserting the placeholder can be done at a similar time in the process to the suggested solution here ✨ Unwrap astro-islands and astro-slots Active
3. It seems that we need bespoke solutions for both SDC and JS components. I don't know if there is a way to generalise this implementation so that the process can happen no matter the ComponentSource.
   

When editing the Hero component that is bundled with XB

1. Update the values of some of the fields
2. Observe that the preview is updated with those changes
3. Focus into and then focus out of the "CTA 1 link" (autocomplete) field
4. Observe that, while the form values remain updated, the preview reverts back to before the changes made in step 1.

If you view the network request that occurs on blur you can see that the data sent as PATCH to the server contains the starting values of the form, not the updated ones.

jessebaker → made their first commit to this issue’s fork.

When a request to the back end for the preview HTML is made currently the HTML returned contains HTML comments to denote the location of regions and slots. When a slot or the main content "region" is empty, the design calls for an empty space into which users can drop their first component to start making the page. The space is styled to have either a green or purple border. This border is rendered in the overlay. Inside the iFrame we simply need some blank space.

The blank space is created by inserting a div with a height and width. In head, we find the HTML comments and insert the div on the client side. This leads to a "jump" after the page is rendered as the new divs are inserted. If the placeholder div were inserted server side the jump would not happen.

So, when a request returns the preview HTML (e.g. a POST or PATCH to /xb/api/v0/layout/xb_page/1 I'd like the HTML returned to already have placeholder divs in between the HTML comments.

For empty regions:

In the markup an empty region currently looks like

<!-- xb-region-start-content --><!-- xb-region-end-content -->

I would like for it to look like this instead:

<!-- xb-region-start-content --><div class="xb--region-empty-placeholder"></div><!-- xb-region-end-content -->

As soon as there is anything rendered in the region, the xb--region-empty-placeholder should not be rendered.

For empty slots

In the markup an empty slot currently looks like

<!-- xb-slot-start-d42bf579-4554-48e2-9438-b859e00d29d8/content -->
            <!-- xb-slot-end-d42bf579-4554-48e2-9438-b859e00d29d8/content -->

I would like for it to look like this instead:

<!-- xb-slot-start-d42bf579-4554-48e2-9438-b859e00d29d8/content --><div class="xb--slot-empty-placeholder"></div><!-- xb-slot-end-d42bf579-4554-48e2-9438-b859e00d29d8/content -->

As soon as there is anything rendered in the slot, the xb--slot-empty-placeholder should not be rendered.

In addition - the server currently renders the first of a slot's examples from the SDC's yml into the slot. This shouldn't be happening. Currently the front end has to remove this markup.

slots:
  column_one:
    title: Column One
    description: The contents of the first column.
    examples:
      - <p>This is column 1 content</p> <--- this should not be rendered in XB

On the front end

Once the backend is inserting those DIVs the front end can then remove ui/src/hooks/useRenderPreviewEmptySlotPlaceholders.ts and ui/src/hooks/useRenderPreviewEmptyRegionPlaceholder.ts

jessebaker → created an issue.

Fixed and merged!

As this is just phase one, I have hidden multi select behind the isDevMode flag which can be enabled by installing the "Experience Builder dev mode" Drupal module.

Multi select currently allows for selecting multiple components at once but does not yet allow a user to perform "bulk" actions like dragging multiple or copy, save, duplicate etc.

Adding https://www.drupal.org/project/experience_builder/issues/3481658 ✨ When zooming the canvas, update the scroll pos. to center on the mouse cursor Active as a related issue - would be nice to get the mouse cursor centering in along with this change!

Addressed feedback on the first review, would like a approval on those updates

Adding demo gif to summary.

jessebaker → created an issue.

jessebaker → created an issue.

RE: #4

1. Why it was an issue to have sidebars on top of canvas ? It was/is still possible to drag and zoom.
It is (and will remain) possible to pan/drag/zoom even with the new "locked" sidebars. The change was to free up more space on smaller screens. We wanted to make better use of the limited space the experience builder UI should take up on the page by reducing the large gaps around the panels. This change also addresses some usability issues with the browser scroll bar being on the right-hand side of the page, right of/behind the right panel.

2. Will it be still possible to see desktop and mobile at same time ? It's quite handy to have both at the same time.
This is unlikely to still be possible however the current UX designs allow for a quick switch between different sizes. Once we show only one viewport size at a time I would like to (at some point) make the iFrame manually resizable. The changes are for 2 main reasons
a) When you have two views side by side, quite often the content doesn't line up. Once the content on a site is fairly long, the mobile view tends to be significantly longer than the desktop due to being much narrower and as a result you can't easily view the same piece of content in both viewports at the same time anyway
b) performance of rendering the site multiple times (with our overlay that shows all the borders etc on hover) was starting to be a limitation.

3. Any plan to make right sidebar (maybe left too) resizable ? I can image a prop with ckeditor could need lots of space for example
Yes! See point 9 in the summary of 📌 [Meta] Application "Shell" design refactor Active and have a sneak peak at some of the designs (still subject to change)!

Further to the above, this is just one step towards a fairly big redesign of the overall layout that is more future proof because it frees up space and sets in place a number of UI patterns (such as the new side menu) that will allow us to more easily add more features over time.

I'm marking this as postponed.

We do not yet have a solution for:

"When errors happen in the rendering of a React component, they can be hit many many times in quick succession. This will send a call to the server for each and every error. I'm not sure what the solution to that is"

I think that solution will take some considerable effort to reach. Rate limiting or some how batching identical errors will be required to ensure we don't accidentally spam the server with requests which will only exacerbate any error the user is already facing!

In addition, once in production, the errors from JS will be pointing to minified/obfuscated JS files which means they will likely be, if not entirely useless, very hard to interpret/action.

Based on conversation with @balintbrews and @callumharrod

1) In this initial approach (which is not sufficient for a high quality DX) we should aim to insert a hardcoded script tag at the top of the preview iFrame that sets a property on the window object to identify that the page is being loaded in XB (window.isInsideExperienceBuilder).
2) For now the expectation is that more complex use cases (like tabs or sliders) will have to be coded for by the component developer making use of that window.isInsideExperienceBuilder. Unlike Layout Builder, XB users will have access to the Layers Panel which will hopefully allow them to, at least, "muddle through" these use cases.
3) In the future a more expansive API will likely need to be developed (or a different solution entirely that we have yet to think of) that allows the preview to maintain data between re-renders. This might be something the component developer will need to manually do (set a state when a change happens and get/check for a state on initialising their JS).

Unfortunately I don't think it's feasible for us to implement 3 right now as we push towards 1.0 even though I agree with @luke.leber that ultimately that is desirable.

Thanks @bnjmnm for the test fix.

jessebaker → created an issue.

Marking as fixed based on @bnjmnm's latest comment. I agree with Ben's response - I think perhaps there is confusion because there is more than one than one React component in this file.

Thanks all! Merging

jessebaker → made their first commit to this issue’s fork.

@luke.leber I agree that there are two parts to this

1) the technical side of how to make a component's JS aware it's in the preview so that it can behave differently (as coded by the component creator)
2) the UX side of how we expect a site builder user to work with/build/preview interactive components in XB.

I'll escalate this with the relevant people on XB team.

jessebaker → made their first commit to this issue’s fork.

jessebaker → created an issue.

jessebaker → created an issue.

Adding 🐛 Components with heigh in VH can cause the preview iFrame to expand infinitely in height Active as a related issue. That fixes a bug that was present for vh units in inline styles.

The stylesheet based VH unit issue described in the summary still exists.

Whoops, this was my bad, I totally missed out the section functionality when implementing the new overlay with Dnd kit

jessebaker → made their first commit to this issue’s fork.

I've created a new issue to track the Code Component related issue that @lauriii is seeing. ✨ iFrame swapper needs to more intelligently wait on Code Components before swapping Active

This issue/MR is specifically targeting a React implementation issue where the React component that renders the iFrame was displaying a single frame with the incorrect height/position calculations before immediately showing the correct frame. It affected both the position of the borders in the overlay and the height of the iFrame itself. It occurs regardless having Code Components or SDCs on the page. It is/was significantly more obvious the slower your CPU.

Before: (4x CPU slow down in Chrome Dev tool enabled)

After: (4x CPU slow down in Chrome Dev tool enabled)

jessebaker → created an issue.

@lauriii I pushed further changes that should stop the height of the iframe jumping up between renders which I think is what is happening in your gif.

jessebaker → created an issue.

It does look like the docs are slightly incorrect in this case.

I think it should read

- `slots`: an object of `slot node`s representing each `component slot` of this `component instance`

If the slots are not returned in the JSON layout data then it would be pretty arduous on the front end to have to recurse through every component instance in the layout and reference the component from the main component list to check if it should have slots and recreate them in the JSON.

@inwerpsel thank you so much for your incredibly in depth and detailed description and the work to identify areas of concern. In this specific case, I think the issue was caused by swapping in the iframe from display: none; to display: block; however I think you have probably identified numerous other places where performance can be improved.

Would you be able to raise a new issue with your observations but with a more general target of improving front end performance over all as I fear that this valuable report will get lost as a comment on this particular issue?

balintbrews → credited jessebaker → .

It seems that something in the act of publishing is losing data from the JSON

Before publishing, the layout JSON for the One Column component (returned from http://xb.test/xb/api/layout/xb_page/2:GET) looks like:

[
  {
    "nodeType": "component",
    "type": "sdc.experience_builder.one_column",
    "uuid": "177122af-1679-4ee4-b700-dcf5ab376c4a"
    "slots": [
      {
        "id": "177122af-1679-4ee4-b700-dcf5ab376c4a/content",
        "name": "content",
        "nodeType": "slot",
        "components": []
      }
    ],
  }
]

after publishing, the slots[] array is empty:

[
  {
    "nodeType": "component",
    "uuid": "177122af-1679-4ee4-b700-dcf5ab376c4a",
    "type": "sdc.experience_builder.one_column",
    "slots": []
  }
]

"code components — or any component that loads JS - XB preview iframe-awareness by checking it's loaded in a iframe[data-xb-iframe]"

I don't think it's possible from inside the iFrame to check properties of the parent iFrame (E.g. a data-xb-iframe attribute). So it would be possible to determine that the code is running in an iFrame but not possible to detect that it's specifically running in XB's iFrame.

But, we could inject a variable from the parent or set a value in localstorage or a cookie that the JS in the iFrame could check for? We would need to do this both for the main preview but also the xb-code-editor-preview iFrame shown in the top right when editing Code Components.

This does, however, "explicitly inform the component it's in a preview" which @wim leers I know you were wary of doing.

jessebaker → created an issue.

jessebaker → created an issue.

I'm marking this postponed. Unless there is a really simple solution to get this working quickly, I think time is better spend replacing the layers with DnD Kit which in general has proven to be a much nicer/smoother/more accessible library.

jessebaker → created an issue.

balintbrews → credited jessebaker → .

Confirmed that exposing the Sidebar region in Olivero no longer breaks the page layout before anything has been added. Merged.

The MR !883 should fix the issue for inline vh units.

I’m not sure what the solution for elements with vh units set in a separate stylesheet would look like. To access the style with JS means calling getComputedStyle() but that always returns the height in px regardless of what unit was specified. Perhaps some kind of rate limit on the iframe height check? Perhaps parsing stylesheets before the page is rendered somehow?

jessebaker → created an issue.