Skip to content
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

Bump Wagtail to 6.4 #40

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Bump Wagtail to 6.4 #40

wants to merge 6 commits into from

Conversation

laymonage
Copy link
Member

Fixes #30.

@laymonage laymonage marked this pull request as draft February 26, 2025 14:53
@laymonage
Copy link
Member Author

Note: this doesn't work yet because we need to replace the alternative_text field in CustomImage with the built-in description field, and replace the custom ImageBlock with the built-in ImageBlock (or subclass it to only add the caption block).

@laymonage
Copy link
Member Author

alternative_text -> description is now done. The template can now be started with Wagtail 6.4. Only the image blocks need to be migrated, but that's optional (TBD).

@laymonage laymonage self-assigned this Feb 26, 2025
required=False,
help_text="If left blank, the image's global alt text will be used.",
)
image = WagtailImageBlock()
caption = blocks.CharBlock(required=False)
Copy link
Member Author

@laymonage laymonage Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried migrating the project's ImageBlock to use Wagtail's ImageBlock but it would result in this very ugly set of panels:

the image label being duplicate four times

This is because the model has a StreamField called image that consists only of this ImageBlock. If we use Wagtail's ImageBlock in place of ImageChooserBlock, we'll end up with another layer of StructBlock that has yet another block called image underneath it.

I think I'll just skip this one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, here's how the block currently looks (with ImageChooserBlock):

image

So, not great either, but it looks even worse with ImageBlock.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a pretty ugly mashup of panels. We may want to revisit the drawing board on ImageBlock in general for this project.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to share that we encountered a similar issue in updating Wagtail.org to the new ImageBlock. This won't be a blocker for the website because the alt text benefits outweigh the visual weirdness. But maybe we should set up an official issue on the Wagtail repo and try to get this fixed in core?

Screenshot 2025-03-05 at 4 42 10 PM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but semantically it makes sense for it to be this way. The first "Image" is the label of the group (struct block), while the second is the label for the image chooser (hence it has the required asterisk).

Handling it in core would mean we do some sort of special-casing with a logic like "If the label of the first block in a nested struct block is the same as the label of the struct block itself, hide the struct block's label", which might be a bit too much magic.


search_fields = AbstractImage.search_fields + [index.SearchField("alternative_text")]
search_fields = AbstractImage.search_fields + [index.SearchField("description")]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AbstractImage doesn't include description in the search_fields for some reason (was that intentional?)

@@ -35,8 +36,7 @@ class Migration(migrations.Migration):
('focal_point_height', models.PositiveIntegerField(blank=True, null=True)),
('file_size', models.PositiveIntegerField(editable=False, null=True)),
('file_hash', models.CharField(blank=True, db_index=True, editable=False, max_length=40)),
('alternative_text', models.CharField(blank=True, help_text="Provide a text alternative for this image for visually impaired users.<br />For advice and best practice, visit <a href='https://moz.com/learn/seo/alt-text' target='_blank' rel='noreferrer noopener'>https://moz.com/learn/seo/alt-text</a>", max_length=200)),
('collection', models.ForeignKey(default=wagtail.models.collections.get_root_collection_id, on_delete=django.db.models.deletion.CASCADE, related_name='+', to='wagtailcore.collection', verbose_name='collection')),
('collection', models.ForeignKey(default=wagtail.models.get_root_collection_id, on_delete=django.db.models.deletion.CASCADE, related_name='+', to='wagtailcore.collection', verbose_name='collection')),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's changed:

@@ -1080,6 +1080,7 @@
"collection": 1,
"title": "Placeholder Image",
"file": "original_images/wagtail.webp",
"description": "A Pied Wagtail walking with its left foot in the air. Copyright The Hall of EINAR.",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found out that the image is from https://www.thehallofeinar.com/2022/03/pied-wagtail/, and there's a copyright watermark. We should probably replace the image with something in public domain, e.g. from Wikimedia Commons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used this one before because it was taken in the UK. The license requires attribution and a link to the source. So we'll have to add those if we decide to use this one: https://commons.wikimedia.org/wiki/File:Wagtail_-_geograph.org.uk_-_6524549.jpg

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can find ones that don't require attribution, such as this one: https://commons.wikimedia.org/wiki/File:White_wagtail_(Motacilla_alba).jpg. Using an image with the least restrictive license is probably more important than where the photo was taken.

@@ -1100,6 +1100,7 @@
"collection": 1,
"title": "frank",
"file": "original_images/frank.png",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know where the image is from 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me either. We might want to replace it with an image from the bakery demo to be safe.

@@ -1122,6 +1122,7 @@
"collection": 1,
"title": "mackerel",
"file": "original_images/mackerel.webp",
"description": "A school of Pacific Mackerel swimming in dark waters.",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems to be from https://cma.recreation.parks.lacity.gov/marine-life/southern-california-species/fish/pacific-mackerel. Again we should probably replace this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct that we should replace this. State and local governments can hold copyrights in the United States. The federal government, however, cannot and all their images are in the public domain unless a copyright by the photographer is cited. NOAA's fish pictures were all too small though.

This jack mackerel image could be a good substitute. Again we would have to provide attribution. https://commons.wikimedia.org/wiki/File:Pacific_Jack_Mackerel_School,_2007.jpg

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discovered today that there actually are NOT more mackerel in Kent than people in China. Not the biggest priority but something to be aware of: https://media.mcsuk.org/documents/Mackerel_no_longer_sustainable_choice_says_environmental_charity.pdf

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +64 to 65
{% if page.image.0.value.caption %}
<p class="text-sm leading-6 py-5">{{ page.image.0.value.caption }}</p>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I had an earlier commit that replaces this with figure and figcaption, but I removed it because we might want to do it in #41 instead. I don't know why we write the template for the image block directly on the page template instead of the image block's template.

Maybe because the image block is meant to be more general-purpose rather than a big main image? Or maybe because the code originally used a foreign key and was replaced with a StreamField that only contains the ImageBlock 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what if people want to add an ImageBlock to their StreamField? Would we encourage them to add images in their article body with RichTextBlock instead? We have to think about how editors are adding images to their content further down the line and I feel like it's slightly more intuitive to select an image block from the StreamField menu than to add it via another method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it just occurred to me that the default ImageBlock could be called into StreamField. I think the argument for having a separate one though would be more precise control of the styling and how the image is displayed.

@@ -125,7 +125,7 @@ class Meta:

class FeaturedArticleBlock(blocks.StructBlock):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block isn't used anywhere, it's imported in news/models.py but not used. I'm not sure how the project template is supposed to be used. Do we only provide the block classes, and users get to choose which ones to include in the StreamField definition? Most of the blocks don't have meaningful templates too (#38).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to create a template for this block because I feel like it's a valuable one to show people how to use and how to incorporate into their projects.

When I went to shoot a tutorial on the starter kit, I was thinking I would have to add the extra blocks myself and surprised to find a bunch of extra blocks without templates or malfunctioning templates were included.

I think this one is worth fixing up. I've gone ahead and provided fixes for several others listed in #38 as well.

@laymonage laymonage requested a review from thibaudcolas March 4, 2025 14:07
@laymonage laymonage marked this pull request as ready for review March 4, 2025 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

Wagtail v6.3 support
2 participants