🇬🇧United Kingdom @jessebaker

I am having a hard time coming up with a robust solution on the client side.

There are two different issues.

The first issue is concurrent editing from two different client instances. That can be either two different users or the same user in two different tabs.

This is solved by the autoSaves hash being passed back and forth and checked and a warning being issued in the instance that a conflict/mismatch arises. ✅

The second issue is proving more difficult to tackle. This issue occurs when a single user in a single client is able to trigger the mismatch scenario by making multiple requests in quick succession.

The issue happens in the following scenario:

1. The client sends an update to the server with the current autoSaves hash (we’ll call it 1).
2. While that request is still in flight, the client sends another update to the server. This request also has autoSave hash 1 because the first request has not returned yet with the updated hash.
3. The server receives the first request and generates a new autoSave hash (2)
4. The response to the first request comes back to the client and the local autoSave hash is updated to 2.
5. The server receives the second request (which has autoSave hash 1) and rejects it (because it should be 2!) as 409
6. The response to the second request is received by the client as 409 and the user is shown an error.

Potential solutions (to problem 2)

1. “Lock” the UI when a request is in flight so that subsequent requests cannot be sent
2. Silently discard new requests made when a request is in flight (this is what Claude’s attempt is doing in !MR1080)
3. Mark in flight requests as "aborted" when a subsequent request is made. When an aborted response returns, discard the returned data knowing that the second response is on the way.
4. Create a client-side “pending” system. While a request is in flight, subsequent data updates to the layout/model are stored as a local variable. When a response is received the local variable is checked and if the variable is set, generate a new request with the data from the variable combined with the autoSaves hash from the most recent response. Then Immediately unset the variable. If multiple data updates are made before the response comes back, they can each just overwrite the local variable as each change is a cumulative data that includes all previous data updates.
5. On loading the React app generate a uuid to use as a client instance ID. Alongside the autoSave hash also send the client instance ID to the server. When the server receives consecutive requests with the same client instance ID, bypass the autoSave hash check as we know that they have come from a single instance and thus can’t be conflicting concurrent changes.

I've investigated the above solutions and have the following conclusions:

1. This has poor UX implications. If we visually indicate the UI is locked it appears flickery on a good connection. If we just silently ignore user actions it makes the app feel unresponsive
2. This allows the server and client to get out of date. The user makes 3 actions but the 2nd and 3rd are silently discarded. It looks like the actions have been made, but on the server only the 1st actually happened.
3. I spent a long time implementing this only to find that it doesn't solve the issue. The subsequent request still has the out of data autoSave hash at the point it is dispatched so it still leads to a 409 error.
4. I have spent even longer attempting to create this system. After 2 days I've not been able to successfully get this working with RTK query. I DO think it's possible, it's just VERY difficult and is resulting in a large amount of additional complexity
5. I have just had this idea and I'm writing this comment on the issue because I'd like to get a second opinion on if its feasible. I think it would work and is less complex than the pending system above.

Thanks @omkar-pd and good catch @mayur-sose! Merged.

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

MR !1106 takes aspects of what was done before and refactors to account for multiple selection and also persists the expanded/collapsed state of the layers in local storage.

jessebaker → changed the visibility of the branch 3479977-expand-layers to hidden.

Blocking work ✨ Move "Dynamic Components" into the Library Active has landed.

jessebaker → changed the visibility of the branch 3479977-tree-expand-logic to hidden.

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

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

There are no changes to review in the MR, all previous changes have been reverted so something has gone awry here I think. Can you take another look @drupalbabaji

RE: #10 I've made a follow up because this MR is already a bit busy 🐛 [PP-1] Reduce unnecessary api calls when opening Components list Active

RE: #11 I think the UX of the library is still evolving and I've seen newer designs that don't even separate Components/Dynamic Components/Code Components and instead have them all in a single list (with filtering options) and even a "folder" structure based on categories. For now I will leave in the order described in #4. It's trivial to change at a later point if we want to.

jessebaker → created an issue.

jessebaker → created an issue.

Just for clarity, are you referring to Exposed Code Components in #3 @lauriii? Clicking Code Components that have not yet been exposed already does open the editor.

RE #7 I believe we do already request the full list of components on page load. The loading state should really not be seen by users unless they load the page on a slow connection and IMMEDIATELY switch to the library tab.

The loading state is more for the benefit of things like our Cypress E2E tests that move much faster than a human!

I've raised ✨ Clicking an item in the library should NOT insert it into the page Active as a follow up based on findings I made while working on this.

jessebaker → created an issue.

jessebaker → created an issue.

🎉

jessebaker → created an issue.

jessebaker → created an issue.

+1 to using existing components and slots and just adding flags!
+1 to the declarative choice of "segments": ["belgium", "uk", "spain"] over the imperative "active": true

I really like the "After personalising" example but I do think it can be simplified (perhaps this is the "optimized further" that you allude to).

In this example, instead of both a personalization-wrapper and a personalization-variant component, there is just a personalization-wrapper and it has a "mutuallyExclusiveSlots": true, flag. Then it is the slots of that component that contain the "segments": ["belgium", "uk", "spain"], information.

...
{
  // 💡Wrapped in a `personalization.wrapper` component, which will contain the different variants.
  "nodeType": "component",
  "id": "uuid-here-personalization-wrapper-yo",
  "type": "personalization.wrapper",
  "mutuallyExclusiveSlots": true,
  "slots": [
    // 💡The first variant
    {
      "nodeType": "slot",
      "id": "uuid-here-personalization-wrapper-yo/variant/default",
      "segments": ["belgium", "uk", "spain"],
      "components": [
        // 💡this is the original!
        {
          "nodeType": "component",
          "id": "baf231e8-b214-4e3e-93d3-5d3f03a1eae9",
          "type": "sdc.experience_builder.druplicon",
          "slots": []
        }
      ]
    },
    // 💡The second variant
    {
      "nodeType": "slot",
      "id": "uuid-here-personalization-wrapper-yo/variant/2",
      "segments": ["uk", "finland"],
      "components": [
        // 💡this is the other version
        {
          "nodeType": "component",
          "id": "baf231e8-b214-4e3e-93d3-5d3f03a1eae9",
          "type": "sdc.experience_builder.xbicon",
          "slots": []
        }
      ]
    }
  ]
}

I have spotted a potential scenario, though, that means that perhaps mutuallyExclusiveSlots is not the correct name for the flag! In the above example an audience member in the UK might expect to see both variants at the same time and thus those are not mutallyExclusiveSlots but rather conditionalSlots.

However, I do think there is a need for the mutallyExclusiveSlots and that is when the purpose of the variant is A/B testing. Where a site author wants 50% of their audience to see one variant and the other 50% to see the other. So perhaps both flags are needed?

RE #2: I think they are just alphabetical.

I have seen future design ideas (that have not yet been approved) that show the components and dynamic components merged into the same part of the library so I think this part of the UI is still subject to change.

Given that it is likely to change, as part of this, I will place Dynamic components next to Components and above Code.

When building/designing this "chip" that shows the variant wrapper, consideration must be made for how we intend to show/handle "selecting components that are stacked one inside the other and take up the same space"

It might be possible to address both at the same time if we can get designs that incorporate both things.

As an example of a competitor's solution to the "components taking up the same space" here is a screenshot of Webflow showing the parent's of a selected component:

Site Studio also has a solution that's quite neat (but I think could be improved upon). I can try and find a screenshot of that if it's helpful.

Closing this as it's no longer an issue once ✨ Refactor Layers UI to use DnDKit Active lands.

I've updated the proposed resolution in the summary to have a lot more detail and improve the UX based on conversation with @callumharrod

jessebaker → created an issue.

I don't really understand how but, @wim leers, your change in !1055 to add the slot placeholder on the server side is working for both SDC and Code Components. I've tested and double checked and the div rendered in ComponentTreeHydrated.php is showing for both types of component.

Those two MR's combined have 100% resolved this issue as far as I am concerned. I've removed the now redundant JS (and 1 other function I found that was already redundant from function-utils.ts (!1055 diff).

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.