-
Notifications
You must be signed in to change notification settings - Fork 541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New ImageBlock with contextual alt text #492
base: main
Are you sure you want to change the base?
Conversation
fd04062
to
2811418
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
bakerydemo/recipes/blocks.py
Outdated
("text", CharBlock()), | ||
("numeric", FloatBlock()), | ||
("rich_text", RichTextBlock()), | ||
("image", CaptionedImageBlock()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it’d be more interesting to use ImageBlock
directly so we can see it in action without caption-related fields.
("image", CaptionedImageBlock()), | |
("image", ImageBlock()), | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, do you think I should remove the CaptionedImageBlock()
entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
bakerydemo/recipes/blocks.py
Outdated
("text", CharBlock()), | ||
("numeric", FloatBlock()), | ||
("rich_text", RichTextBlock()), | ||
("image", CaptionedImageBlock()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Thanks! @Stormheg |
{ | ||
"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 | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These models are new, should these be part of the fixtures? Asking @laymonage or @gasman for input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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? |
Fix for the |
Thanks for that pointer! @Stormheg |
@Chiemezuo I wanted to test drive wagtail/wagtail#11791 using this branch but I got an error importing the fixtures with
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 |
I'll look into it and give you feedback in about 2 hours or so. @Stormheg |
@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. |
This will be the modifications for the bakerydemo project to align with the changes from PR #11791.