New ImageBlock with contextual alt text #492

Chiemezuo · 2024-07-26T10:26:14Z

This will be the modifications for the bakerydemo project to align with the changes from PR #11791.

thibaudcolas

Solid! I’ve suggested one minor change. Note we will need for wagtail#11791 to be merged and released before we can merge this for bakerydemo, as the demo site also has to support stable versions of Wagtail.

thibaudcolas · 2024-08-02T09:18:25Z

bakerydemo/recipes/blocks.py

+            ("text", CharBlock()),
+            ("numeric", FloatBlock()),
+            ("rich_text", RichTextBlock()),
+            ("image", CaptionedImageBlock()),


I think it’d be more interesting to use ImageBlock directly so we can see it in action without caption-related fields.

Suggested change

("image", CaptionedImageBlock()),

("image", ImageBlock()),

Oh, do you think I should remove the CaptionedImageBlock() entirely?

I think CaptionedImageBlock still has a right to exist. It would allow us to test the ImageChooserBlock, something which is now not possible anymore as it has all been changed to ImageBlock.

Perhaps we can allow ImageBlock to be used directly and still have the CaptionedImageBlock somewhere else?

Stormheg

Great work so far @Chiemezuo - makes it a lot easier to review your changes from wagtail/wagtail#11791 👍 👍

I have a comment about still using ImageChooserBlock in some places so we can continue using bakerydemo to test that separately from the new ImageBlock.

Also, we discussed updating the fixtures with initial data. Could you populate the contextual alt text fields for the pages now using ImageBlock and generate new fixtures to reflect the changes? Currently, trying to save a blog pages results in an error because there is no contextual alt text set. Updating the fixtures will resolve that.

As @thibaudcolas mentioned, we cannot merge this until ImageBlock makes it into a stable version of Wagtail. So lets keep this marked as draft for now.

Stormheg · 2024-08-05T12:15:29Z

bakerydemo/recipes/blocks.py

+            ("text", CharBlock()),
+            ("numeric", FloatBlock()),
+            ("rich_text", RichTextBlock()),
+            ("image", CaptionedImageBlock()),


I think CaptionedImageBlock still has a right to exist. It would allow us to test the ImageChooserBlock, something which is now not possible anymore as it has all been changed to ImageBlock.

Perhaps we can allow ImageBlock to be used directly and still have the CaptionedImageBlock somewhere else?

Chiemezuo · 2024-08-06T06:23:14Z

Thanks! @Stormheg
I've added the fixtures. Just to be sure if I got things right, can you help me check again on your end?

Stormheg · 2024-08-06T07:05:42Z

bakerydemo/base/fixtures/bakerydemo.json

+  {
+    "model": "wagtailadmin.editingsession",
+    "pk": 1,
+    "fields": {
+      "user": ["admin"],
+      "content_type": ["wagtailcore", "page"],
+      "object_id": "82",
+      "last_seen_at": "2024-08-06T01:35:42.544Z",
+      "is_editing": false
+    }
+  },
+  {
+    "model": "wagtailadmin.editingsession",
+    "pk": 2,
+    "fields": {
+      "user": ["admin"],
+      "content_type": ["wagtailcore", "page"],
+      "object_id": "82",
+      "last_seen_at": "2024-08-06T01:38:47.215Z",
+      "is_editing": false
+    }
+  },


These models are new, should these be part of the fixtures? Asking @laymonage or @gasman for input.

If we want to add new models, that should be done in a separate PR. For this PR, we should filter the fixture changes to only the ones relevant for the PR.

Stormheg · 2024-08-06T07:16:09Z

@Chiemezuo I see you added the fixtures, but I'm still missing initial data for the contextual alt text fields.

http://localhost:8000/admin/pages/77/edit/

Could you populate these fields and regenerate the fixtures?

Stormheg · 2024-08-06T07:23:48Z

Fix for the build-container workflow here: #495

Chiemezuo · 2024-08-06T07:48:25Z

Thanks for that pointer! @Stormheg
Done!

bakerydemo/recipes/blocks.py

bakerydemo/base/blocks.py

Stormheg · 2024-08-28T08:26:59Z

@Chiemezuo I wanted to test drive wagtail/wagtail#11791 using this branch but I got an error importing the fixtures with manage.py load_initial_data

  File "/Users/storm/projects/open-source/wagtail/bakerydemo-chiemezuo/venv/lib/python3.12/site-packages/wagtail/blocks/stream_block.py", line 392, in get_searchable_content
    content.extend(child.block.get_searchable_content(child.value))
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/storm/projects/open-source/wagtail/bakerydemo-chiemezuo/venv/lib/python3.12/site-packages/wagtail/images/blocks.py", line 77, in get_searchable_content
    return super().get_searchable_content(value)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/storm/projects/open-source/wagtail/bakerydemo-chiemezuo/venv/lib/python3.12/site-packages/wagtail/blocks/struct_block.py", line 275, in get_searchable_content
    block.get_searchable_content(value.get(name, block.get_default()))
                                 ^^^^^^^^^
AttributeError: Problem installing fixture '/Users/storm/projects/open-source/wagtail/bakerydemo-chiemezuo/bakerydemo/base/fixtures/bakerydemo.json': 'Image' object has no attribute 'get'

I have this revision from wagtail/wagtail#11791 checked out and installed: wagtail/wagtail@ac86fe7

Could you have a look please? Not sure what is the cause of this.

Edit: seeing the same error when I manually save a page

Chiemezuo · 2024-08-28T08:34:22Z

I'll look into it and give you feedback in about 2 hours or so. @Stormheg

Chiemezuo · 2024-08-28T12:36:23Z

@Stormheg Looking at it right away, I suspect I know the problem. It's from one of the last changes I did at the last moment.
@thibaudcolas felt the get_searchable_content could be reworked and I attempted with the parent class' method. I've replicated and fixed the issue on the PR #11791.
You can check it again.

modifies BakeryDemo guide for new ImageBlock

2811418

Chiemezuo force-pushed the update/for-new-image-block branch from fd04062 to 2811418 Compare July 26, 2024 10:34

Stormheg self-requested a review July 29, 2024 12:10

thibaudcolas changed the title ~~modifies BakeryDemo guide for new ImageBlock~~ New ImageBlock with contextual alt text Aug 2, 2024

thibaudcolas approved these changes Aug 2, 2024

View reviewed changes

Stormheg reviewed Aug 5, 2024

View reviewed changes

add fixtures to branch

aacdadf

Stormheg reviewed Aug 6, 2024

View reviewed changes

update alt text for dessert page

445cd0d

thibaudcolas reviewed Aug 8, 2024

View reviewed changes

bakerydemo/recipes/blocks.py Show resolved Hide resolved

thibaudcolas reviewed Aug 8, 2024

View reviewed changes

bakerydemo/base/blocks.py Show resolved Hide resolved

rename CaptionedImageBlock template

9983305

thibaudcolas mentioned this pull request Aug 28, 2024

New ImageBlock for StreamField wagtail/wagtail#11791

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New ImageBlock with contextual alt text #492

New ImageBlock with contextual alt text #492

New ImageBlock with contextual alt text #492

Are you sure you want to change the base?

New ImageBlock with contextual alt text #492

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment