🇬🇧United Kingdom @jessebaker

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

I'm going to close this - I think ultimately the context menu and it's position is 99% 3rd party code coming from the Radix library and we have to assume they have their own tests.

In the original implementation of our canvas there was a quirk that meant the scaling was somehow introducing errors in the positioning logic but I don't see that we will be going back to that implementation and a test is only really going to be testing that some hypothetical future change isn't going to introduce an unknown bug.

I'm not sure it's possible to actually get into those situations (yet?!) but good catch. At the moment, you can't even access XB if you don't have permission to edit XB pages so if you can access XB then you will always have at least one option in the menu.

Anyway I've added some more robustness around the UI when people have no permissions or can delete things but don't have any other items in the menu which should keep this looking tidy in the future no matter what configuration of permissions people end up with!

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

Rather than just removing the option, I took it upon myself to make a basic version of this work as it may help with a11y etc.

Now the "Move Into" menu item displays a sub menu that contains a list of slots belonging to the siblings of the component in context. Clicking one will move the component into the chosen slot.

The functionality is basic but offers a basis onto which we can add later improvements as the need arises or if there is input from UX at some point in the future. I can already predict now that you can "move into" it would be helpful to "move out of" - but I don't want to grow the scope more than I already have.

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

Finally got to the bottom of a number of issues that were causing a lot of friction in writing the Playwright tests for this work. All resolved now and ready for another round of review.

The summary on the MR is the most up to date place for all info on this work.

penyaskito → credited jessebaker → .

jessebaker → created an issue.

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

There is a simpler approach to this using our already existing useDebounce hook. Hopefully the comments on the MR explain clearly how to implement it!

I have merged, this can be tested on 0.x now @neha_bawankar

I'm afraid I don't remember why I added the min-height to the dialogs. I have removed it and everything seems to be working find so I'm going to chalk it up to me trying something that I later backed out of but forgot to remove.

In the process of reviewing this, I worked with @callumharrod to refine some of the designs to better account for the weights fields and improve the look of the "remove" button.

In working with him, we decided to change the media items to stack in rows instead of the 3 columns. It provides better readability, a better use of space and accounts for media items with longer file names much better.

I've gone ahead and implemented his suggested changes in this MR. I've also given it some pretty thorough testing and a code review and it's all looking good!

(I found one bug that is also present in 0.x - if you have a media widget on the page data side bar, pick some images and then switch to a different sidebar and come back the selected images are no longer shown).

wim leers → credited jessebaker → .

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

The format of contentEntityCreateOperations which was merged is useful - it's easy to iterate over to build the list of "New x" buttons in the UI. Your proposal in #16 means, to build the list of "New x" buttons, I would have to iterate over the permissions object, filter to just keys that starts with 'create:' and then split the key by : to get the entity type and bundle info.

Maybe there is some value to having the permissions listed in the permissions object as you propose, but I think only if that was in addition to still having the info in the original format elsewhere e.g. drupalSettings.xb.contentEntityCreateOperations

"contentEntityCreateOperations":{
    "xb_page":{"xb_page":"Page"},
     "node":{"article":"Amazing article"}
   }

I'm slightly too late to the party but I've just merged in upstream changes over at ✨ Create React Permission Utilities Active and the shape of the permissions object is now more complex as a result of this and as a result is going to make the FE code more complex to work around it.

Previously xb.permissions was a simple

interface permissions {
  [key: string]: boolean;
}

This made it easy for the FE to quickly assert if a user had or did not have a given permission by simply checking that list of booleans.

Can this new contentEntityCreateOperations object be moved into the root of drupalSettings.xb instead of inside the permissions object?

jessebaker → created an issue.

@larowlan I'm unclear what "this code" is referring to in the following:

Make this code more resilient so that every source plugin doesn't have to conform to a particular twig format in order to get the HTML comments

Do you mean make the code that inserts the HTML comments more resilient (so change that a Source plugin can render its output without using xb_slot_ids and xb_uuid but still have the Twig extension output the comments.) or are you talking about making the FE code more resilient (e.g. handle the situation where the layout/model data suggests there is a slot but the markup does not have comments)?

Marking as postponed pending a bit more info as I am unable to reproduce.

@lauriii Do you think this is a speed/race condition situation? It seems like, in the gif, the issue occurs when quickly clicking the link immediately after refreshing the browser? Do you think that's the reason why it's only happening "in some instances".

For clarity the way the link interception works is that there is a drupal behaviour loaded in the page when it's in the preview that preventsDefault on the click event and uses a postmessage to communicate up to the parent of the iFrame (our React app).

My suspicion is that some kind of race condition either means you are able to click the link before the event handler has been bound to the link OR the event handler is not getting bound to the link as a result of it running before the link is rendered on the page (by Astro).

Embedded screenshot.

Adding related issue https://www.drupal.org/project/experience_builder/issues/3529623 📌 Style Autocomplete suggestions Active
Removing related issue around the extensions dialog - The solution there is unfortunately not applicable here.

Adding related issue - perhaps this can be addressed at the same time?

🐛 Dropdown List in Page Data Area of XB Platform Not Scrollable, Preventing Access to Taxonomy Term Options Active

In all honesty I found this very difficult to reproduce in any kind of consistent way. Following the steps in the video lead to the issue being exhibited maybe 10% of the time.

With my fix (which is a bit of a stab in the dark) I've not been able to reproduce the issue - but I'm not confident I've identified the reason the issue is occuring so maybe it just makes it better but there is still some underlying race condition that could still happen.

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

Thanks @libbna for your offer of helping. I had actually already done most of the work when I created this issue so I've got an MR already. My bad for not assigning it to myself when I created it.

jessebaker → created an issue.

In the interests of keeping things as simple as possible on the FE, I was hoping to get something like the following

JSON:

{
  "drupalSettings": {
    "xb": {
      [...]
      "permissions": [
        "read global regions", "update global regions", "delete global regions", "read sections", "update sections", "delete sections", ...
      ],
    }
  }
}

Because then in the UI checking if we should display a button or not is as simple as, for example:

JSX:

<>
    {drupalSettings.xb.permissions.includes('delete sections') && (
        <button>Delete section</button>
    )}
</>

There are probably unrelated reasons why the client should always call /xb/api/v0/layout/xb_page/1 after publishing.

#20

As of my latest commit the FE now re-requests the page after publishing changes IF the current page is amongst the list of pending changes.

In my testing there is one remaining bug that I can see that is quite tricky to reproduce.

1. In a browser tab start from a blank page, no autosaves (everything published)
2. Copy the URL and open a new tab so you have two tabs with the same blank page state.
3. In Tab 1, add a Heading component and immediately publish all changes
4. Switch to Tab 2.
5. Preview is now out of date (showing blank still, but should have a Heading on it)
6. Add a Hero component
7. There should be a conflict warning but there isn't.

From what I can see, the issue is that the autosave hash has autoSaveRevision but it's always "1". Nothing I do ever seems to increment that number (that is actually a string).

 {
  "autoSaveRevision": "1",
  "hash": "efa8dd3ce4f5f9e1"
}

There are no longer any blockers to this!

jessebaker → created an issue.

@tedbow re: #18 #20

1) I think perhaps we should create/address this in a follow up
2) +1 to using 'changed' or even perhaps 'lastModified' and using a timestamp - I could see that information actually being useful elsewhere in the UI at some point.
3) After publishing I think we already refetch some data so I don't see it being a problem to update the page.

jessebaker → created an issue.

The X in "Review x changes" is currently updated by the GET call to /xb/api/v0/auto-saves/pending

We already call that every time...

1. The user makes a change
2. Every 10 seconds on a poll
3. Every time the "Review x changes" button is clicked and the list popover is opened

While the list is open, the poll is suspended but if a user attempts to publish and they are out-of-sync with the server, the response to that will update the X again.

Is there some scenario where this is not adequately keeping that number up to date that I'm missing?

wim leers → credited jessebaker → .

@tedbow I've found a bug that I think needs to be addressed server side.

Steps

1. Ensure you have no autosaves/pending changes
2. Reload the page to ensure you start fresh
3. Make a change (e.g. edit a component prop)
4. Make another change. Note that these changes both make requests with the correct/latest autoSave hash in the payload
5. Publish all changes
6. Make a third change. Note that this request correctly sends the last autoSave hash that was received in .4
7. The response to this third change is a 409.

I suspect the latest autoSave hash on the server side is either being deleted or updated when changes are published but not communicated back to the FE so the client ends up with an out of date hash.

I think that either a) publishing should not update the hash, or b) the response from a publish action needs to include the updated hash to the FE can store it for the next request.

jessebaker → created an issue.

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