jessebaker → made their first commit to this issue’s fork.
jessebaker → made their first commit to this issue’s fork.
I've resolved this specific issue but there is still work to be done here - deleting pages via the navigator vs the pages list have slightly different UX/entirely different paths through the code! I've create a follow up #3551462: Centralise React code for deleting pages →
@hooroomoo I've just added some steps to reproduce to the summary.
Marked as needs review but I've not waited on the tests to pass because it's the end of my day. I don't imagine there will be an issue, but who knows!
jessebaker → made their first commit to this issue’s fork.
I suspect that the test failing on the first MR is as a result of the test SDC I dropped in so I've made a 2nd MR that contains just the fix.
When reviewing, it should be ok to review the first MR so you have the test SDC but approve/merge the 2nd.
jessebaker → made their first commit to this issue’s fork.
Ok, ignore the previous comment, the fix DID remove 1 case of flakiness. The continued flakiness of this test is actually because of an intermittent bug and NOT a problem with the test.
Reported bug here: #3550786: Publishing then exposing and then using a code component too quickly results in failed hydration →
It seems my hope for a quick fix did not result in the test no longer being flakey - it literally flaked the first two times I've run it (or maybe I've broken the test entirely somehow!?).
I have not got a working AI set up locally so leaving unassigned in the hope that someone else can take a look who is able to debug the test locally.
I think this is going to be about as robust as we can get it when deleting the currently edited component. The previously edited path is obviously very transient because if they came straight to the code editor (or reloaded the page at any point) it won't exist.
Thankfully this work, in addition to the UX changes in https://www.drupal.org/i/3546119 → should make this a lot less problematic. Once 3546119 lands the user will be able to immediately navigate using the sideMenu from the /canvas/editor route.
I made Broken components (components where component.broken === true have a ⚠️ icon, show as disabled and prevented them from being dragged or right clicked.
jessebaker → made their first commit to this issue’s fork.
jessebaker → made their first commit to this issue’s fork.
jessebaker → made their first commit to this issue’s fork.
jessebaker → changed the visibility of the branch 3540965-add-a11ytest-labels to hidden.
I've managed to find a bug - see https://git.drupalcode.org/project/canvas/-/merge_requests/139#note_597085
jessebaker → made their first commit to this issue’s fork.
jessebaker → made their first commit to this issue’s fork.
Because even though Video 1 is clicked in step 2, its form hasn't loaded yet.
I don't think that's correct. Firstly, Video two has a width of 150 (it's default width), not 190. Secondly, immediately before setting the width, the Duck video was chosen in the media browser and correctly applied to video 1 in the preview. So the correct form was displayed when the Duck video was chosen and the correct element in the preview was updated but immediately after that, the width was typed in and was not sent to the server (an empty array was sent instead).
I'm not sure there is actually much value in landing this "temporary" fix ASAP vs working though and solving the underlying problem. When looking at the Before/After gif on the MR description it took me several loops of both gifs to even see the difference. The whole form content still flickers - it's just that the white background no longer does.
I think merging this just muddies the waters and creates more overhead as we'll need another follow up anyway and that will have to unpick the work in this MR.
When an entity is deleted, shouldn't the suggestedEntityId be updated to point to a different node that does exist? Or does that happen already and this is to catch the case where the entity was deleted but the UI hasn't been refreshed yet (or if the user visits an out of date url with the id in the params?)?
jessebaker → made their first commit to this issue’s fork.
I've approved and merged everything so far. I suggest leaving this issue open until the end of our sprint as a bucket to catch any other small things that come up.
jessebaker → made their first commit to this issue’s fork.
This seems to have been caused by adding the line aria-label={`${item.name} preview thumbnail`} in ui/src/components/list/ListItem.tsx. Removing the aria-label prop resolves the issue (but will introduce a regression in the a11y check Playwright test).
It seems that putting aria-label on the Radix Tooltip <Tooltip.Content /> does some shenanigans where it renders a hidden div inside the tooltip with the label text in and this is somehow interfering with the height calculation?
Had a little play around with this. It seems like drag/drop in the UI is generating a POST to canvas/api/v0/layout-content-template/node.article.full/3 with the correct Layout/Model JSON (you can see the order has changed) but no new autosave is generated (note no changes button in the top right doesn’t update) and the preview is refreshing but the markup doesn’t reflect the change.
What is stranger still is that if I just have 3 components on the template and re-arrange the order of them that works fine, it only seems to stop working once you have a component with populated slots.
jessebaker → made their first commit to this issue’s fork.
jessebaker → made their first commit to this issue’s fork.
jessebaker → made their first commit to this issue’s fork.
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 → created an issue.
jessebaker → created an issue.
jessebaker → made their first commit to this issue’s fork.
jessebaker → made their first commit to this issue’s fork.
I spotted a small bug in the Manage Library tab: SDCs and Block components have cursor: default on hover but JS Code components have cursor: grab;
Next steps:
Now that Drupal is no longer aware of which entity is being edited because the Route is handled by the React SPA we need another way of getting the drupalSettings.canvas.entityTypeKeys
@bnjmnm suggested instead loading the keys for all the different entityTypes (keyed by the entity type) https://git.drupalcode.org/project/canvas/-/merge_requests/4#note_581234
Before this MR it was possible to do this
const titleInput = entity_form_fields[`${canvasSettings.entityTypeKeys.label}[0][value]`];
With the change above we would be able to do
const titleInput = entity_form_fields[`${canvasSettings.entityTypeKeys[entityType].label}[0][value]`];
More context
(Probably too much context, but in case it's useful)
drupalSettings.canvas.entityTypeKeys was previously based on the current entity being edited and looked something like
{
"id": "id",
"uuid": "uuid",
"revision": "revision_id",
"label": "title",
"langcode": "langcode",
"published": "status",
"owner": "owner",
"bundle": "",
"default_langcode": "default_langcode",
"revision_translation_affected": "revision_translation_affected"
}
We would take the value of entityTypeKeys.label (in the above example "title") and that would let us know how to get page title from the entity_form_fields which look like e.g.
"entity_form_fields": {
"langcode[0][value]": "en",
"title[0][value]": "Untitled page",
"path[0][alias]": "\/untitled-page",
"path[0][langcode]": "en",
"image[media_library_selection]": "",
"description[0][value]": ""
},