🇬🇧United Kingdom @jessebaker

jessebaker → created an issue.

Unpostoned now that ✨ Focus mode for global regions Active has landed. Will need quite a bit of work to re-factor since upstream has moved on a lot.

mglaman → credited jessebaker → .

Clicking publish all should publish changes. It currently doesn't, but it will at least show you an error as to why it can't publish.

There are follow ups

1. As per @tedbow's suggestion in his review: polish handling for errors moved to follow up ✨ Polish flow for Publish all changes" when receiving conflict errors in response Active
2. Publishing currently fails on nodes due to an error with the URL alias field 🐛 Unable to create URL aliases with XB enabled Active
3. Changes show up immediately when previewing a node in XB 🐛 Once previewed in XB an entity with no changes will still show up in "Review x changes" Active
4. The pending changes API endpoint should list individual regions for global template changes. Currently adding content to a node will show two changes - one for the node and another for the template[#3500390]
   

Added related issue and updated summary.

Added ✨ Polish flow for Publish all changes" when receiving conflict errors in response Active to SHOULD-HAVE for 6. Save (draft) content

jessebaker → created an issue.

wim leers → credited jessebaker → .

I just found it now because I'm implementing the "Publish all changes" button and the tests (that I'm writing) for that cannot pass because it's currently not possible to publish changes due to this issue!

balintbrews → credited jessebaker → .

This is also happening for the URL Alias field. I've added a gif to the summary.

cy.findByLabelText should continuously retry (for up to 5 seconds I think is the default) until it finds an element with the text it is looking for. I think the error would be something like "could not find element with the text Image" if that was the issue.

In the log it says uncaught exception TypeError: Cannot read properties of undefined (reading 'removeAttribute') which appears to be an error coming from JS (cypress fails immediately on JS errors). I wonder if Cypress is being too hasty and trying to interact with an element that is being re-rendered - but the click is happening on an element that got removed from the dom (because of the re-render).

A potential solution would be to split the check and the click action. But honestly it's a strange one and I don't know if this would actually make a difference.

cy.findByLabelText('Image').should('exist');
cy.findByLabelText('Image').realDnd('[data-xb-slot-id="content"]', {
    position: 'top',
});

I have ranked the follow ups and added them to 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active to be tracked and added a gif of the new functionality to the summary.

tedbow → credited jessebaker → .

Assigning to me to continue from @soaratul

Adding follow up issues from Global Regions MR

I've just done a fresh install on 0.x after merging in the global region work and I'm no longer seeing the issue.

I'm still working in this area. Will update this issue if I come across the issue again or have better steps to reproduce.

Merged. 🎉

Follow ups created:

• ✨ Non-empty global regions need to be highlighted when hovering with the green color Active
• 🐛 Global region focus spotlight is inaccurate sometimes Active
• 🐛 Drag and drop in Layers is difficult to drop into bottom position Active
• 🐛 Drag and drop in Layers flickers when trying to drop into an empty slot Active
• 🐛 Dropping a component outside of the focused region breaks the preview Active
• 🐛 Dropping a component in a global region does not update the preview Active

jessebaker → created an issue.

jessebaker → created an issue.

jessebaker → created an issue.

jessebaker → created an issue.

jessebaker → created an issue.

jessebaker → created an issue.

I'm going to merge this and then create follow ups to address the outstanding issues. The MR is too unwieldy to keep alive due to the size of it! Watch this space for new issues.

RE: #22

1. Resolved. I've also made it so that double clicking the focused region in the layers will take you back out again so you don't have to move your mouse all the way up to the top nav to move between regions.

2. Looks like the Page nav got merged in after this was ready for review. I've correctly reconciled that - and fixed the size of the Page drop down to match Figma while I was here!

3. The margin/size of the region is taken into account. The issue you are showing in your screenshot is that the breadcrumb block has CSS with negative margins to make it appear outside of the region it is rendered in. I'm not sure I can think of a solution for that?

jessebaker → created an issue.

Approved and merged! Good catch

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

I've approved and merged !577 which resolves #24.1 and #24.2

Closing this issue. There are two follow ups for #24.3 and #24.4

📌 The pending changes API endpoint should list individual regions for global template changes Active and
📌 Decide how to assign colours to users for "review changes" Active

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

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

I have added a number of todos to the codebase in my work on [#34997648] - whoever works on this should search for #3499364 in the React code. They are places where I've needed to add work arounds for the HTML wrapper being different for regions that are not the content region.

I've taken a (quick) look at this and hit a bit of a blocker - storing the map in Redux doesn't seem to work. I wrote a slice (which I've pasted below as a starting point should someone else want to pick this up. Maybe I just made a silly mistake, but it seems that fundamentally storing references to the DOM elements in Redux isn't the way to go.

ChatGPT suggests: While it's technically possible to store a reference to a DOM element in Redux state, it's generally not recommended. Redux state is intended to be serializable and should ideally only contain plain JavaScript objects, arrays, strings, numbers, and booleans.

import { createAppSlice } from '@/app/createAppSlice';
import type { PayloadAction } from '@reduxjs/toolkit';
import type {
  ComponentsMap,
  SlotsMap,
  RegionsMap,
} from '@/types/AnnotationMaps';

export interface PreviewElementMapSliceState {
  regionsMap: RegionsMap;
  componentsMap: ComponentsMap;
  slotsMap: SlotsMap;
}

const initialState: PreviewElementMapSliceState = {
  regionsMap: {},
  componentsMap: {},
  slotsMap: {},
};

type setRegionsMapPayload = RegionsMap;
type setComponentsMapPayload = ComponentsMap;
type setSlotsMapPayload = SlotsMap;

export const previewElementMapSlice = createAppSlice({
  name: 'previewElementMap',
  initialState,
  reducers: (create) => ({
    setRegionsMap: create.reducer(
      (state, action: PayloadAction<setRegionsMapPayload>) => {
        state.regionsMap = action.payload;
      },
    ),
    setComponentsMap: create.reducer(
      (state, action: PayloadAction<setComponentsMapPayload>) => {
        state.componentsMap = action.payload;
      },
    ),
    setSlotsMap: create.reducer(
      (state, action: PayloadAction<setSlotsMapPayload>) => {
        state.slotsMap = action.payload;
      },
    ),
  }),
  selectors: {
    selectRegionsMap: (maps): RegionsMap => {
      return maps.regionsMap;
    },
    selectComponentsMap: (maps): ComponentsMap => {
      return maps.componentsMap;
    },
    selectSlotsMap: (maps): SlotsMap => {
      return maps.slotsMap;
    },
  },
});

export const { setRegionsMap, setComponentsMap, setSlotsMap } =
  previewElementMapSlice.actions;
export const { selectRegionsMap, selectComponentsMap, selectSlotsMap } =
  previewElementMapSlice.selectors;

Before spending time on a rebase, the code changed in this issue is being heavily refactored over in ✨ Focus mode for global regions Active .

I think I can cherry pick the relevant code from this MR and add credit for @manojmakrasu over there. Or else, this should wait until that issue lands before being updated to save unnecessary work.

jessebaker → created an issue.

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

Using the same numbers as #22

1. The Figma isn't immediately clear. Is it correct that the categorization is as follows
   • Pages - grey, page icon (Although home page is show with a House icon)
   • Global Regions - green, cube icon (Figma also shows the diagonal 3 square icon in some places and refers to them as Global Sections)
   • Components - purple, diagonal 4 square icon
   • Elements - grey, various icons (we don't have a concept of elements yet)
2. Should be a simple fix. I think the code currently falls back to the component icon for types it doesn't know and does not have a case for 'node'.
3. I believe this will need a change to the data the endpoint returns - perhaps better suited to a follow up issue.
4. This poses many questions. Where does the colour come from? Can users choose a colour? Is the colour stored somewhere or should it be randomly generated on page load? Should the colour come back as a property of the user and be determined on back end? What are the available colour options? Again, maybe a follow up for this.

RE: #14

I think this should be postponed until either

• 📌 Create extendibility proof of concept that also serves as documentation-by-example Active lands and we can programmatically call Redux actions like "insertNodeAtPath" or "moveNodeToPath" or
• OR some kind of more accessible UX is implemented that allows e.g. components to be added to slots via clicking, without drag+drop (which is alluded to in this Figma or see screenshot below )

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

larowlan → credited jessebaker → .

Very late to this party, but just adding a +1 to @bnjmnm's comment in #46 about being hesitant to use an iFrame. I think iFrame really should only be the approach of last resort. Looking at the progress since that comment, I'd say @bnjmnm is on to a winner as that's looking great and avoids the fragility of iFrame back and forth.

Seeing the progress so far I'm optimistic the CSS scoping will work as a robust enough solution here. Nice 👏

Postponed on ✨ Leverage HTML comment annotations, remove wrapping div elements Active

I have added some docs to the MR and also created a follow up ✨ Refactor React Context (ComponentHtmlMapContext) to Redux to allow for better extensibility Active to perform a small refactor from Context to Redux that was not deemed important enough to hold up this MR any longer.

jessebaker → created an issue.

Closing as dupe of ✨ Leverage HTML comment annotations, remove wrapping div elements Active

Created followup issue 📌 Implement HTML comment annotations for Regions, replace HTML wrappers Active to do the same conversion for wrappers around Global Regions

jessebaker → created an issue.

Approved, merged.

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

jessebaker → created an issue.

jessebaker → created an issue.

jessebaker → created an issue.

I don't think is particularly valuable to have an end to end test that ACTUALLY drags and drops elements (e.g. mousedown, mousemove, mouseover, mouseup etc) . It is a lot of work, is prone to failure etc. And we use a 3rd party library that I assume has its own tests anyway. Furthermore it's immediately obvious to any human that it is broken as it's such a core piece of the XB functionality so it's unlikely to get missed for any length of time.

Instead, I wonder if we can find a way to programmatically call the same reducer function that occurs on the drop event directly from Cypress. This would ensure the same code runs (updating the JSON, sending the JSON to the server, receiving the updated HTML, updating the preview) without having to actually do all the fiddly stuff of emulating mouse moves and hover events etc.

Steps to reproduce are as simple as:
Create new node
Enter XB
Drag+drop the One column component into the Content region

I've done a full reinstall, drush cr etc. Using Olivero with the XB Global Regions enabled in the theme settings.

Full stack trace from logs is as follows:

TypeError: Cannot assign null to property Drupal\experience_builder\Plugin\DataType\ComponentTreeStructure::$tree of type array in Drupal\experience_builder\Plugin\DataType\ComponentTreeStructure->setValue() (line 124 of /Users/jesse.baker/Sites/drupal11xb/web/modules/contrib/experience_builder/src/Plugin/DataType/ComponentTreeStructure.php).


Backtrace	
#0 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/TypedData/TypedDataManager.php(216): Drupal\experience_builder\Plugin\DataType\ComponentTreeStructure->setValue('null', false)
#1 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php(123): Drupal\Core\TypedData\TypedDataManager->getPropertyInstance(Object(Drupal\experience_builder\Controller\HardcodedPropsComponentTreeItem), 'tree', 'null')
#2 /Users/jesse.baker/Sites/drupal11xb/web/modules/contrib/experience_builder/src/Plugin/Field/FieldType/ComponentTreeItem.php(199): Drupal\Core\TypedData\Plugin\DataType\Map->get('tree')
#3 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php(135): Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem->onChange('tree', false)
#4 /Users/jesse.baker/Sites/drupal11xb/web/modules/contrib/experience_builder/src/Plugin/Field/FieldType/ComponentTreeItem.php(222): Drupal\Core\TypedData\Plugin\DataType\Map->set('tree', 'null', false)
#5 /Users/jesse.baker/Sites/drupal11xb/web/modules/contrib/experience_builder/src/Controller/ApiPreviewController.php(141): Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem->setValue(Array)
#6 /Users/jesse.baker/Sites/drupal11xb/web/modules/contrib/experience_builder/src/Controller/ApiPreviewController.php(66): Drupal\experience_builder\Controller\ApiPreviewController->clientLayoutAndModelToXbField(Array, Array)
#7 [internal function]: Drupal\experience_builder\Controller\ApiPreviewController->__invoke(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\node\Entity\Node))
#8 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Object(Drupal\experience_builder\Controller\ApiPreviewController), Array)
#9 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/Render/Renderer.php(593): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#10 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(121): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#11 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Object(Drupal\experience_builder\Controller\ApiPreviewController), Array)
#12 /Users/jesse.baker/Sites/drupal11xb/vendor/symfony/http-kernel/HttpKernel.php(183): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#13 /Users/jesse.baker/Sites/drupal11xb/vendor/symfony/http-kernel/HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#14 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/StackMiddleware/Session.php(53): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#15 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#16 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/StackMiddleware/ContentLength.php(28): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#17 /Users/jesse.baker/Sites/drupal11xb/web/core/modules/big_pipe/src/StackMiddleware/ContentLength.php(32): Drupal\Core\StackMiddleware\ContentLength->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#18 /Users/jesse.baker/Sites/drupal11xb/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(116): Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#19 /Users/jesse.baker/Sites/drupal11xb/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(90): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#20 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#21 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#22 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php(36): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#23 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(51): Drupal\Core\StackMiddleware\AjaxPageState->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#24 /Users/jesse.baker/Sites/drupal11xb/web/core/lib/Drupal/Core/DrupalKernel.php(709): Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#25 /Users/jesse.baker/Sites/drupal11xb/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#26 {main}

jessebaker → created an issue.

I was thinking that annotating attributes could be achieved by something like the following with a comment immediate preceding an element describing which props map to which attributes.

<!-- xb-start-8db0a067-742c-4356-acdb-241cf3304e37 -->
<div>
  <p>
    <!-- xb-attrs cta_url:href|cta_target:target -->
    <a href="https://www.example.com" target="_blank">Link example</a>
  </p>
</div>
<!-- xb-end-8db0a067-742c-4356-acdb-241cf3304e37 -->

I can only think of use cases where we would want to highlight the element on which an attribute exists and this would have enough info to do that.

However, I am certainly interested in @longwave's suggestion of using some kind of separate map that comes along with the HTML. The only way I can see that falling down is similar to the issue described in #11 where the DOM is manipulated by JS library after the JSON map has been generated and thus the paths get out of sync.

In this MR so far I'm focusing purely on removing the wrapping divs around each component and the divs wrapping each slot and replacing them with HTML comments. In doing this, most of the refactor is around a) making sure the Overlay UI still matches the size of the elements it is supposed to be overlaying and b) making sure sortableJs still works for dropping components into the iFrame.

Potentially, now that the MR for adding global region support has landed, support for global regions via comments instead of div will be added in this MR too but it might make sense to push that to a follow up issue. I'm still looking into it.

Adding comments around individual prop values is something we will need to address later and I believe the main technical challenge there is the work to add those comments on the server - potentially necessitating generating/leveraging and AST of the Twig files.

My solution in #20 (and the current state of the MR) is that some data attributes are added specifically to elements rendered by components that can be drag/dropped to enable SortableJs to function but they are added at run time (in JS) after parsing HTML comments and not added on the backend.

I think there are multiple factors at play here.

1. Each component is wrapped in a div.xb--sortable-item even if it has CSS to make it display: none. This means it is receiving the extra margin-block-end that @larowlan mentioned in the issue summary. I believe this will be solve once things are wrapped with HTML comments instead of divs.
2. Each region is wrapped in a div.xb--sortable-list. I'm not sure what the intent of that is at the moment, but in future you will only ever be dragging/dropping into one "focused" region at a time. ✨ Focus mode for global regions Active

I have just checked on my branch where the wrapper divs have been replaced with HTML comments the spacing appears the same in XB as it does looking at the front end of the same node as a logged out user:

I think that means, this is not an XB issue, but perhaps an issue with the theme?

Ah, that was it, I can now replicate. Thanks, I'll take a look

Yeah I have the regions all showing up and working in the layers panel - but they are all empty.

In your screenshot (although it's mostly cropped off) it looks like you have a number of components inside several of your regions.

I'm struggling to reproduce this.

I created a new node and added an SDC (hero) to the Content region.

In the iFrame there is a just one div.region that contains my hero SDC. There are no empty div.region elements. Any region that does not contain a component is not rendered at all.

Looking at your screenshot @larowlan it looks like you have selected a .xb--sortable-item. That div should be wrapping a component which would suggest that the region you are looking at is not in fact empty.

Looking at your issue @wim leers if you can reproduce it can you confirm that the div.xb--overlay-region__higlighted div has a height that is different to the height of the div.region inside the iFrame? (also looking at the markup in your screenshot I can see inside the region there are multiple div.componentOverlay elements which would suggest that your highlight region is not empty either.

Can either of you share a screenshot of the left hand layers panel when this issue is occuring - I'd like to see what components that UI thinks are inside the highlight region.

Great debugging @balintbrews. I'm glad my hunch was at least along the right track and helped out some.

This is a really nice collaboration - nice work everyone.

balintbrews → credited jessebaker → .

Wrapping up for the weekend - my work is not complete here but here is where I'm at.

I pivoted slightly from my two different approaches described in #11.

Now, each time a new document is rendered in the iFrame, I run two walker functions that iterate over the document and 'parse' the HTML comments into two maps, one for components and one for slots. The mapping allows me to very easily find the HTMLElement in the iFrame for any given component UUID or slot ID.

They look like this:

interface SlotInfo {
  element: HTMLElement;
  componentUuid: string;
  slotName: string;
}

interface ComponentInfo {
  elements: HTMLElement[];
  componentUuid: string;
}

export type SlotsMap = Record<string, SlotInfo>;
export type ComponentsMap = Record<string, ComponentInfo>;

Where the string/key in each Record is the ID or UUID of the slot or component.

In practice this allows me to call componentsMap[uuid].elements and retrieve an array of the DOM elements that make up that component (and do similar for slots).

This doesn't immediately solve the issue described where the DOM may be manipulated after the map is built to remove the original element but it works as well as the current 'wrapping divs' solution and will be easy enough to extend to make it capable of running a fallback to catch those kind of situations.

In less positive news, what I have done in the refactor so far seems to have broken the ability to drag and drop so there is still some way to go before this is complete. Hopefully I can polish it off before holidays!

I'm not sure your alternative approach in #16 @lauriii would be enough to handle the scenario of disabling an auto-playing image slider for instance.

Maybe it's just semantics but "Custom component code should not have to be aware..." - suggests that components must work without being aware they are in the editor but doesn't say that they can't be told they are in the editor if that allows for someone to improve the DX when building them out.

I think we have work arounds planned to mean that any/all components will work and be useable (e.g. falling back to using the layers panel) which fulfils that requirement but we shouldn't intentionally hamper more advanced use cases just to meet that requirement.

So we are now generating comments server side, and parsing those comments and turning them into data attributes into the client?

😅 - that's basically true, yeah!

The benefit of handling on the client side is that the browser has everything we need to manipulate the DOM easily. I'm sure the same things are possible on the back end but, long term, I do think that leveraging the HTML comments "in real time" as and when we query for components/slots is the preferred solution. Handling this on the front end for now means that the later refactor will be more easily handled without having to make both PHP and JS changes.

My initial approach to this problem was to build out an api/library similar to JS's native DOM querying methods (e.g. getElementById) that would allow me to do things such as getComponentElementByIdInHTMLComment or getSlotByHTMLComment. This would mean that it was possible to replace document.querySelector etc with one of the above and hopefully lead to a fairly easy to implement solution.

However:

1. building out that library is proving tricky in itself (but I do have some parts working)
2. after having built several parts of it, there is such a level of complexity already in the code for e.g. building the overlay components, that adding this additional layer of abstraction makes the code feel very unwieldy to work with.

The advantage of my initial approach is that it means that we don't have to touch the user's mark up at all and thus it should (in time) be robust and able to handle any and all situations. The downside is the addition of significant complexity in lots of places.

I have started to look into a different approach however. This second approach is far simpler. As the HTML for the preview is retrieved from the server we can modify it (parse it to a DOM document and then use JS to modify the DOM) before we hand it over to the preview renderer.

The advantatge of doing it this way is that I can take each HTML comment and easily convert it to data-xb- attributes on the corresponding HTML elements. This means things like document.querySelector(['data-xb-slot']) become available, simplifying the implementation everywhere.

The downside of this new approach is that there a small number of scenarios that we won't be able to support. An example of that is:

Let's say someone has a JS library to render a video. The server outputs an XB component that renders a <video> tag but also attaches a JS library. The JS library, on load, will find that video tag and replace it with an

Hmm, that's a really difficult challenge.

There are actually 2 iframe elements one on top of the other. Each time any data changes, the hidden iframe is updated with the new preview and once it has loaded, the iframes are swapped so you can see the changes.

Because the page is re-rendered server side whenever there are any changes, the iframe swapping has to be done to hide the document loading and stop any FOUC issues/flickering.

As a result of that implementation though, no front end state on the page is maintained between data updates.

Off the top of my head I don't have an immediate thought of how to even begin addressing that.

I actually snuck into 0.x a very work-in-progress/hacky (and totally untested by anyone except me) feature where you can press and hold the 'V' key and it will hide the overlay UI and allow you to interact with the page inside the iFrame. *

I use it for debugging and I have some vague intentions to build it out into a full feature in the fullness of time, but I'm curious if it is enough to work around your problem (and as such would be a good use case for us to consider more fully fleshing out that feature).

As for your request, when I was building Visual Page Builder for Site Studio we had to add in a variable/property to let Site Studio interactive components know they were being rendered in the VPB for very similar reasons to the ones you are facing. I think one of the main reasons was a gallery slider that would auto-transition to the next slide every few seconds which was very undesirable when trying edit the slides!

* if you get stuck in the Visual mode, make sure you focus outside the iFrame and press 'v' again a few times and it will unstick you. The issue is the keyup event when you let go of V sometimes gets swallowed by the iFrame. Like I said, very work-in-progress!

jessebaker → created an issue.

jessebaker → changed the visibility of the branch 3491021-leverage-html-comment to hidden.

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

In going through and trying to implement this I have consistently hit a pain point in working with the data formatted as described in #6 and #7

Without going into huge details about the painpoint I'm hitting, I think @wim leers suggestion in #5 to give each slot an id (changed from uuid) of 'id' => $component_instance_uuid . '/' . $slot_name, will go a long way to fixing it. In simple terms, it will allow me to specify a specific slot without having to always keep track of the slots parent when recursively traversing the tree.

So I think that would be:

export interface RegionNode {
  nodeType: 'region';
  name: string;
  components: ComponentNode[];
}

export interface ComponentNode {
  nodeType: 'component';
  uuid: string; // unique uuid
  type: string; // corresponds to the drupal component id e.g 'sdc.experience_builder.druplicon'
  slots: SlotNode[];
}

export interface SlotNode {
  nodeType: 'slot';
  id: string; // made up of the parent components uuid and the slot's name.
  name: string;
  components: ComponentNode[];
}

From there, potentially one other QoL change for the front end would be to normalise 'uuid' on components and 'id' on slots to both be 'id' in the JSON but that is not critical and perhaps the clarity of giving them the correct names is more important.

I agree with everything in #5, #6 and #7.

This will require a lot of refactoring on the front end but as @longwave says this is solvable and I do think is the correct path forward.

In fact I think there are already assumptions on the front end that components can only contain slots and slots can only contain components already, it's just not explicitly stated/defined in TS or anything. E.g. In ComponentOverlay.tsx I loop over component.children and for each one I render a <SlotOverlay />. In turn the children in SlotOverlay.tsx are all rendered as <ComponentOverlay />s.

I agree with everything in #5, #6 and #7.

This will require a lot of refactoring on the front end but as @longwave says this is solvable and I do think is the correct path forward.

In fact I think there are already assumptions on the front end that components can only contain slots and slots can only contain components already, it's just not explicitly stated/defined in TS or anything. E.g. In ComponentOverlay.tsx I loop over component.children and for each one I render a <SlotOverlay />. In turn the children in SlotOverlay.tsx are all rendered as <ComponentOverlay />s.

Not saying we shouldn't follow up at all, just that it's a bit premature at this point as the data model is in the process of changing.

At the moment root = green, component = blue, slot = purple but with the new data model we will have a data model that represents regions, components, elements, slots and the current page/content area so all the styling and the data it is based on will be changing. I'm not sure it makes sense to have a specific issue just for changing the colours now rather than just applying the right colours once we have the right data model underpinning it all.

I think once the data model is in, it will be trivial to attribute the right colours to the right things and then we can go through with UX with working examples to iterate on.

jessebaker → created an issue.

The root concept is going away once there is more than one region and will be replaced with a 'content' region - I think perhaps that can be named after the region. I made it green for now as it's closest to what a region will be. Blue = component, purple = component slot and green = region. I think that's correct based on the designs.

Following a meeting between @wim leers, @longwave, @f.mazeikis and I, I created the following notes:

1. At the top level, we will have an array of regions - always at least a `content` region which will replace ‘root’ concept.
2. We should default the tree to having a ‘content’ region on everything. It’s special cased and will error if it doesn’t exist.
3. Permissions handled back end - regions a user doesn’t have access to won’t be sent (but the HTML in the preview will still be present).
4. Suggestion: Always send full tree from client to server to enable editing more than one region in a single ‘round trip’
5. Validation: backend can return unique identifier - front end can use that to highlight the errored “thing” where ever it might be
6. Later: discuss with @Lauriii the repercussions/design behind what things do/don’t go into the temp store. I think the consensus was that pretty much everything should always save to temp and be published in one action at a later point to avoid things like a pattern existing but it contains components that were never published.
   

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

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

I've taken the pragmatic approach to merge this as it stands. The code in this MR is a step forward that allows a user to save albeit in a fairly unpolished way from a UX and code quality standpoint. However, it is not intended to be the final iteration of the Publish workflow so I think it's worth having merged for now despite those shortcomings.

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

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