🇬🇧United Kingdom @jessebaker

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.

jessebaker → created an issue.

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

Just adding a comment for extra visibility in case my comment on the MR was missed.

Would you be able to change the HTML comments so that there is a difference between a comment wrapping a slot and a comment wrapping a prop?

jessebaker → created an issue.

longwave → credited jessebaker → .

jessebaker → created an issue.

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

jessebaker → created an issue.

📌 Allow saving component compositions as sections Postponed has landed!

Closing - the improvements to these functions was adapted and included as part of ✨ Allow copy pasting components with CTRL+C and CTRL+V Active which already landed.

Thanks @shyam_bhatt I took what you implemented and ran with it - it was really useful. I fixed up the tests and made a few further changes.

jessebaker → created an issue.

jessebaker → created an issue.

jessebaker → created an issue.

I'm leaving [PP-2] in the title, however, I was able to make progress on the front end side of this ticket and have raised an MR that can be merged even though ultimately clicking "Save to library" won't actually work yet - it does allow you to get all the way to that pont!

Great work @soaratul thank you! Marking fixed.

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

jessebaker → created an issue.

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

If we do want to keep the test from this MR, the test needs to be almost totally re-written since 🌱 [Proposal] A different approach to the iFrame preview interactions Active landed.

The new contextMenu (since the iFrame overlay was implemented) uses the Radix ContextMenu component which handles the positioning of the menu internally (and one would assume they have tests for this already). It's rendered a level above the one where the canvas is scaled.

However we should ensure that a future refactor/code change doesn't suddenly render the contextMenu at "the wrong level" such that it becomes affected by the scaling so there is a valid case to still implement a test here.

This needs a little more work, see code review on the MR.

Thanks for the speedy fix. I've raised a follow up feature request ✨ Automatically select the next component on deleting a component Active

jessebaker → created an issue.

jessebaker → created an issue.

This is currently working with one minor visual issue that looks pretty bad. After pasting the overlay/outline UI flashes up, unstyled/broken.

I think this is ready for review, but I will continue looking into this last visual issue. Depending on the scale of the fix, I will either push a further commit to this branch or create a new issue.

I've not got any further in providing steps to reproduce this, but it did happen to me just now and I was able to deduce that the issue is occuring in ui/src/features/layout/previewOverlay/ViewportOverlay.tsx. It seems that updateRect() is either not being called, or being called and failing to correctly update the rect.

I've raised an MR that I hope will be more reliable, but it's hard to be sure given I'm not sure the exact sequence of events that cause the issue.

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

I think you are correct @soaratul - the default browser behaviour means that the autoscroll is only invoked when dragging and the mouse cursor is < ~15px from the edge of the window. As our top bar and zoom controls are 20px from the edge, then making them have pointer-events:none actually has no impact on the autoscroll.

Closing as outdated because other changes since this ticket was opened have meant the issue is no longer valid.

jessebaker → created an issue.

jessebaker → created an issue.

Taking this on as I need to use replaceUUIDsAndUpdateModel in the work on Copy/Paste that I'm doing.

I went ahead and resolved the conflicts on merging 0.x as it was quite tricket. Doing that has broken the new Cypress test because that still interacts with the iFrame in the 'old' way.

I think going through and updating the Cypress test to replace e.g. cy.getIframeBody().findAllByText('XB Needs This For The Time Being').should('have.length', 2); with cy.clickComponentInPreview('Hero', 1) will get the test working again.

Leaving unassigned if anyone wants to pick up, else I can jump on this tomorrow.

Fixed in 🌱 [Proposal] A different approach to the iFrame preview interactions Active

Since landing 🌱 [Proposal] A different approach to the iFrame preview interactions Active this issue is improved however I do think there is still room for improvement in that when you drag to the top and bottom of the screen, if your mouse is positioned over either the top bar or zoom controls the scroll behaviour is not invoked.

I suspect setting pointer events to none on those elements while isDragging is true will resolve the issue.

Moving to Minor.

Fixed in 🌱 [Proposal] A different approach to the iFrame preview interactions Active

🌱 [Proposal] A different approach to the iFrame preview interactions Active has landed! It is no longer possible to click or interact with links (or anything else) in the preview.

🌱 [Proposal] A different approach to the iFrame preview interactions Active has landed! This is no longer blocked.

🌱 [Proposal] A different approach to the iFrame preview interactions Active has landed!

🌱 [Proposal] A different approach to the iFrame preview interactions Active has landed!

Updating to [PP-1] as 🌱 [Proposal] A different approach to the iFrame preview interactions Active has now landed!

Fixed in 🌱 [Proposal] A different approach to the iFrame preview interactions Active

Fixed in 🌱 [Proposal] A different approach to the iFrame preview interactions Active . Contextual menu now opens in the correct place and is automatically closed on zooming or panning the canvas.

Closing in favour of ✨ [PP-1] Implement client side search for the library Active

Sadly I've just confirmed that this issue still exists after the work to move the iFrame interactions into an overlay ( 🌱 [Proposal] A different approach to the iFrame preview interactions Active ) as it is related to the <IFrameSwapper> and the sortableJS implementation inside the iFrame neither of which have been substantially changed.

In an attempt to push this issue along, I went ahead and added the test described by @bnjmnm in #17. Unfortunately this appears to have highlighted a further problem. Or perhaps this is actually the symptom of the problem @bnjmnm is referring to in that comment.

When props that are NOT the new Integer Enum (select) field are updated, the value of the Integer select field is NOT sent to the back end.

IOW the initial value of the select does not appear to be correctly updated in the redux store when loading the form for the first time.

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

@bnjmnm is really busy with a bunch of other things at the moment. So if anyone else wants to push forward on this (implementing @hooroomoo's suggestion above) please feel free to take it on!

I've done a bit of manual testing and found some issues around entering numbers. Would you prefer to address here, or else I can raise them as new issues?

On a required, string, field (E.g. the Heading field on the default Hero component)

1. Entering a number fails validation but I would expect my entered value to be cast to a string. What if I want a heading that says "911" for example.
2. If I select all the text in an input and press space, the value is immediately changed to a 0 and then the app crashes.
4. Typing in -0 also crashes the app. (possibly same issue as 2)
5. After typing a number, I can't press space (e.g. If I wanted the heading to be "4 person menu"

On a URI field (E.g. the CTA 1 Link field on the default Hero component)

1. Typing 0 crashes the app.
2. After typing a number, I can't press space (e.g. If I wanted the heading to be "4 person menu"

I've merged in the change to fix the "fake sections" preview. @lauriii I'm not sure if this issue should remain open until the actual real implementation of sections is complete or if you are happy that it will be address as part of that work ( 📌 Allow saving component compositions as sections Postponed )

@kristen pol that screenshot → certainly appears to be showing that, however I am not sure it's a good idea or technically necessary. I have a design sync call later today and I'll seek clarification on this.

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

Reviewed, but further changes/refactor needed, thank you.

Postponing this pending the decision on 🐛 Some components cannot be used in XB because UI prevents SDC props being named `name` Active and the code changes from 🌱 [Proposal] A different approach to the iFrame preview interactions Active being merged