🇬🇧United Kingdom @jessebaker

Yup, closing this. #3498819 can pick up the design related follow ups.

jessebaker → created an issue.

I've done most of the refactor that I suggested in #20

The list of pages that is returned from /xb/api/content/xb_page was previously updated to include a key autoSaveTitle . Can the same be done to also return an autoSavePath?

The page title should always reflect the autosave version of the page title.

The page title exists in 4 places -

1. The value in the page data form field
2. The title shown on the dropdown button in the top middle
3. The title shown in the list of pages when you click the navigator
4. The title shown in the list of autosaves in the review changes dropdown.

I think the CURRENT page title can only be updated from two places

• Changing the value in the form field
• Publish all changes in the rare situation that the page was renamed in another tab/by another user

Other pages' titles (shown in the navigator) can be updated in one place

• When publishing all changes

Furthermore and not mentioned thus far in this issue, the navigator also shows the pages' url alias.

So, my proposed solution

1. When user types in page title, in real time, purely on the client-side we update the value in the dropdown button. This is already in place in 0.x
2. When user types in page title (or any field in the form on the right) we update the preview by calling the server This is already in place in 0.x
3. When user types in page title, we invalidate the cache of the pages list (/xb/api/content/xb_page) in the navigator (using invalidateTags)
4. We also invalidate the cache of the pages list when user types in the URL alias,
5. We also invalidate the cache of the pages list when a user "publishes all"
6. On the server side, when fetching the list of pages, the page title and url alias of each page returned should be that of the autosave if there is one.

Then we must ensure that when the navigator is opened, if the cache has been invalidated, we re-fetch the list of pages.

Looks like the Electron renderer crash is more widespread than just this issue so might just be a coincidence

14.0.3 was the first version I tried and that did seem to pass the tests. By the time I came back to merge it, 14.1.0 had released so I updated to that instead. I think the issue we are seeing was introduced in the in 14.1.0. If it's something else, going back to 14.0.3 should result in passing tests.

There isn't any specific feature added that we want/need in the versions since the version we are currently using so this isn't urgent. I didn't realise it would end up being more work than just running an npm command!

Following on discussions on this that happened in meetings:

The Content "region" is not actually a region and it should not be displayed to look the same as the other Global Regions (which are shown in green).

Furthermore, at a future point a page might have more than one Content region (once we start looking at templates exposing XB slots) so we need to account for that.

Oh it might not be out of memory, the error just suggests that it might be memory related. It seems to be pretty consitantly not working on this version of Cypress.

We detected that the Electron Renderer process just crashed.
We have failed the current spec but will continue running the next spec.
This can happen for a number of different reasons.
If you're running lots of tests on a memory intense application.
  - Try increasing the CPU/memory on the machine you're running on.
  - Try enabling experimentalMemoryManagement in your config file.
  - Try lowering numTestsKeptInMemory in your config file during 'cypress open'.
You can learn more here:
https://on.cypress.io/renderer-process-crashed

So the MR already has the update however Electron (the headless browser part) seems to be crashing with OOM errors since upgrading. I don't know if perhaps the newer version is just more memory heavy or if the CI pipeline was having a bad day. I've kicked them off again and we'll see in a short while!

If the OOM errors persist then some more investigation will be needed I guess (and that doesn't need to be me)

Since the original video was recorded the frequency and severity of this flickering has been reduced through other work being merged in.

I'm still, on occasion able to see a single frame flash of grey when updating props (particularly if the page is long and I'm editing something towards the bottom)

It appears that it's possible for the current iframe to be hidden before the replacement iframe has rendered thus showing a single frame with neither iframe visible:

Just linking this here for future reference - some exploration into building a "dropzone overlay" that integrates with our overlay that would go further to prevent us having to interfere with the iFrame document contents

https://jsfiddle.net/marmite/fmop190z/

Merged!

I think this probably got solved along the way as I can't reproduce this nor see anything not working.

RE : #3 I have reproduced this.

The good news is, the issue appears to be in code we control, not in the depths of sortableJS and I think on my first look that it's a visual issue more than anything more fundamentally unsupported.

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

jessebaker → created an issue.

Nice job, thanks

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

hooroomoo → credited jessebaker → .

Adding:

astro-island, astro-slot, astro-static-slot {
  display:contents;
}

breaks both the Overlay and SortableJS meaning you basically can't use XB!

The overlay (for both slots and components) relies on being able to get the boundingClientRect of the components and slots it is mirroring but that doesn't work with display:contents as those elements then have no box size. This can possibly be worked around by detecting that CSS and instead basing the overlay size on the combined size of the children of the element.

SortableJS must have some internal check for the sortable items' size too because as soon as you add display: contents to the sortable items they stop working.

jessebaker → created an issue.

wim leers → credited jessebaker → .

wim leers → credited jessebaker → .

jessebaker → created an issue.

jessebaker → created an issue.

jessebaker → created an issue.

Great stuff, thanks!

Yay, this is feeling much more robust now. Thanks

wim leers → credited jessebaker → .

Haha, it wasn't meant to be mysterious. I made a little test component

  const [height, setHeight] = useState(400);

  useEffect(() => {
    const changeHeight = () => {
      const newHeight = Math.floor(Math.random() * (500 - 100 + 1)) + 100;
      setHeight(newHeight);
    };

    const intervalId = setInterval(changeHeight, 2000);

    // Cleanup interval on component unmount
    return () => clearInterval(intervalId);
  }, []);

  return (
    <div style={{ height: `${height}px`, transition: 'height 0.5s ease', background: '#f00' }}>
      <h2>A Heading Element</h2>
    </div>
  );

hooroomoo → credited jessebaker → .

Merged, added screenshot (you have to peer closely but there is subtle frosted glass-like transparency to the panels now!)

jessebaker → created an issue.

jessebaker → created an issue.

Thanks, merged!

@lauriii yup - that issue will (should) fix that!

jessebaker → created an issue.

I've manually tested this and I'm still seeing an issue with the following steps

1. Update a field on a component - add ' one'
2. observe the preview updates to show 'my heading one'
3. Update the field again - append ' two
4. observe the preview updates to 'my heading one two'
5. Click Undo
6. observe the preview reverts back to 'my heading one' and so does the field.
7. Click Redo
8. observe the preview DOES NOT update back to 'my heading one two' but the field in the sidebar does.

I have updated the undo-redo.cy.js test to check these steps (the test only checked the field values, not the markup in the preview which I assume is how this regression got through in the first place)

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

Merging, thanks!

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

MR670 is branched from MR648 and has the additional FE parts to show and update the status badge in UI.

jessebaker → created an issue.

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

jessebaker → created an issue.

No longer blocked.

jessebaker → created an issue.

GIF to follow in 📌 Render the extensions P.o.C into the Extensions dialog Active once it's all looking a bit nicer!

wim leers → credited jessebaker → .

@wim leers - @longwave has given a +1 in response to your question here https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

This just needs a final review I think and can be merged.

balintbrews → credited jessebaker → .

I merged in 🐛 Image can only be replaced one time before "add media" stops working Active to speed things along, but ideally there should be a test to ensure this doesn't regress. Adding a test to ensure the media can be replaced multiple times in a row should be a requirement of this MR.

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

jessebaker → created an issue.

wim leers → credited jessebaker → .

jessebaker → created an issue.

wim leers → credited jessebaker → .

After some too-ing and fro-ing and meetings I have drawn this which I think captures the states of the Status Button and when each state should be shown.

1. We need some way to determine this and then that info needs to be made available to the React code.
2. As @tedbow said in #2, the response from /xb/api/autosave contains a list of autosave items. Each has an entity type and id that we can compare to the current entity type and id. If there is a match, we know there are changes.
3. Same as 1.

Once we are able to expose the required info for 1 and 3, I don't think the logic on the FE is complex to implement.

I suspect both FE and BE are needed. But before that can happen I do think we need to adjust or agree upon terminology here.

Currently in 0.x the status badge can exist in one of two states. Published or Draft. This is currently based on the status of the node. When creating a new node through XB it is set to Draft but, because it’s based on the status of the autosaved version of the node, as soon as an autosave is created (and the page is refreshed) it will be Published. I suspect this is not your intended behaviour.

Step one: We need a way to differentiate between Drupal’s Publish status and our “Publish all changes”. They are both currently called Publish and it’s confusing. I guess also the badge is referred to as the Status Badge but I don't think the intent is to show the node's status value but actually the state based on XB's autosave state.

Step two: Outline exactly what that badge is supposed to show. And when it should show in each of those scenarios.

Ideally we would have a workflow in Figma that shows the status in each of these steps (there may be more, this is off the top of my head)

1. User creates new page and opens it in XB->User makes a change->user reviews and “publishes all changes”
2. User opens an existing page in XB->User makes a change->user reviews and “publishes all changes”
3. User opens an existing page in XB that has an autosave->User makes a change->user reviews and “publishes all changes”

Additionally, there are weird things like
User opens an existing page->User changes the status to “Unpublished” in the page data form->user reviews and “publishes all changes”

wim leers → credited jessebaker → .

jessebaker → created an issue.

Merged!

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

Merged!

Adding credits.

jessebaker → created an issue.

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