🇬🇧United Kingdom @jessebaker

Account created on 27 September 2017, about 8 years ago
#

Merge Requests

More

Recent comments

🇬🇧United Kingdom jessebaker

jessebaker created an issue.

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker
🇬🇧United Kingdom jessebaker
🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

jessebaker created an issue.

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

jessebaker created an issue.

🇬🇧United Kingdom jessebaker

Assigning to @effulgentsia to create follow up

🇬🇧United Kingdom jessebaker

@hooroomoo I've just added some steps to reproduce to the summary.

🇬🇧United Kingdom jessebaker
🇬🇧United Kingdom jessebaker

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!

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

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.

🇬🇧United Kingdom jessebaker

jessebaker created an issue.

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker
🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

jessebaker created an issue.

🇬🇧United Kingdom jessebaker

🎉

🇬🇧United Kingdom jessebaker

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.

🇬🇧United Kingdom jessebaker

jessebaker created an issue.

🇬🇧United Kingdom jessebaker

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.

🇬🇧United Kingdom jessebaker

I made Broken components (components where component.broken === true have a ⚠️ icon, show as disabled and prevented them from being dragged or right clicked.

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

jessebaker created an issue.

🇬🇧United Kingdom jessebaker

jessebaker changed the visibility of the branch 3540965-add-a11ytest-labels to hidden.

🇬🇧United Kingdom jessebaker

jessebaker created an issue.

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

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).

🇬🇧United Kingdom jessebaker

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.

🇬🇧United Kingdom jessebaker

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?)?

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

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.

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

jessebaker created an issue.

🇬🇧United Kingdom jessebaker

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?

🇬🇧United Kingdom jessebaker

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.

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

jessebaker created an issue.

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

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

🇬🇧United Kingdom jessebaker

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;

🇬🇧United Kingdom jessebaker

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]": ""
    },
Production build 0.71.5 2024