Conversation
… to ImageBlock On this site, most images have pretty decent titles so using them while we are converting from ImageChooserBlock to ImageBlock often produces the alt text you likely would have written anyway. Inserting it during the first page save makes this conversion a lot easier for our editors.
…g images to ImageBlock" This reverts commit 305f1f1.
|
After testing all the blocks where
The benefits of implementing the new block outweigh the issue of having the word image appear twice in my opinion. I think implementing this block to improve our alt text is more important. This issue needs to be properly logged and addressed in future versions of Wagtail Core though. |
|
I've asked @Chiemezuo to rename the migration files so that they better describe the change being made with each migration. |
vossisboss
left a comment
vossisboss
left a comment
There was a problem hiding this comment.
All right @Chiemezuo, after doing my initial testing and review, there are a few things that need to be addressed.
As we discussed on the call, it would be really helpful if all the migration files had a more descriptive change to let future maintainers know what changes were made by the migration.
I also found a few instances where ImageChooserBlock was not updated to the new ImageBlock and I would like to know whether those were deliberate choices or whether there were a few instances you missed. If you could go through those and also search the project files for remaining instances that need to be replaced, that would be great.
I am also curious about the update needed to our custom image model. I'd just like to know a bit more about why it's necessary. And we'll want to test this on staging.wagtail.org for sure to make sure any changes to the model don't lead to database problems with our existing images.
There was a problem hiding this comment.
I assume adding this Field to our custom image model is necessary for the alt text? I thought the new ImageBlock was interchangeable with ImageChooserBlock, so I'm just curious why this change is necessary.
wagtailio/core/migrations/0062_alter_contentpage_body_alter_homepage_body.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks like there is an instance of ImageChooserBlock left in this file that wasn't replaced by the new ImageBlock. Did you not change it because it wasn't changed in the prior PR @Chiemezuo? Or do you have another reason for leaving it be?
See line 109 in the LogoCardBlock.
There was a problem hiding this comment.
Also looks like there is another instance of ImageChooserBlock in line 463 in LogoBlock.
There was a problem hiding this comment.
Looks like there is another instance of ImageChooserBlock on Line 22 under CardBlock. Is there a reason we're not changing this one? If not, we should probably see if the new ImageBlock works well here.
For this, it's a minor change. The AbstractImage model was modified around the same time that the ImageBlock was being created. It adds a 'description' field which will be the primary default for images that do not have alt text. Prior to the new changes, the alt text default was the 'title' field. However, with the new changes, it will be alt text -> description -> title, in that order of defaults. |
vossisboss
left a comment
vossisboss
left a comment
There was a problem hiding this comment.
All right! Testing on local and staging went very well. I discovered one unrelated bug but otherwise I think this will be a good update to our website.
|
One note for us to remember after merging this: The new description fields Chieumezo added will need to be updated for all snippet images since most of those models don't incorporate |
4 participants
This is a follow-up to #480
I removed the patch (and the resulting older migrations) as the issues it addressed were fixed in Wagtail 6.4.
I also rebased so I could preserve the commit authors and their history, with a few additions.