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

Migrate some ImageChooserBlock instances to ImageBlock #480

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Stormheg
Copy link
Member

Commit extracted from #478 (minus migration files to avoid conflicts)

We are not ready to move our usage over yet, as this will immediately result in validation errors when an editor tries to save an existing page with instances of ImageBlock on them, as the contextual alternative text will be missing

This PR is pending a decision on the way forward.

cc @cnk @vossisboss

@Stormheg Stormheg mentioned this pull request Nov 14, 2024
@cnk cnk force-pushed the chore/migrate-imagechoosers-to-imageblock branch 3 times, most recently from 31a36c0 to 8963bf7 Compare November 21, 2024 03:25
@cnk
Copy link
Member

cnk commented Nov 21, 2024

Hmmm perhaps I should remove the commit with the patch to use default alt text. It works great in the browser and most titles I checked were perfectly fine as the image's alt text.

I tried to look into the failing python tests but I can't run them locally. I have a virtual env and tried poetry run ./manage.py test (copied from ci.yml) but I get the same error as in CI for all branches I tried - including main

Stormheg and others added 2 commits December 4, 2024 12:49
… 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.
@cnk cnk force-pushed the chore/migrate-imagechoosers-to-imageblock branch from 8963bf7 to 4fb0f62 Compare December 4, 2024 20:58
@cnk cnk force-pushed the chore/migrate-imagechoosers-to-imageblock branch from 4fb0f62 to af5167b Compare December 4, 2024 21:05
@cnk
Copy link
Member

cnk commented Dec 4, 2024

I rebased this onto current main and whatever was causing tests to fail appears to have resolved itself.

However, I am not getting the behavior I expect when creating a new blocks which contain an ImageBlock. If I choose an image I know has a description, I expected to see the "Alt text" field filled in with that description - like the image chooser in Draftail has done all along. But I am just getting blank fields for 'decorative' and 'alt/description'. If someone can replicate this on their installs, I think I'll raise an 'am I mistaken' issue on wagtail itself. @Stormheg @vossisboss

(Note: I added description text on my local copy of the site; YMMV with other copies of the DB)
Screenshot 2024-12-04 at 3 27 33 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants