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

Add alt_text field to image content type #701

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

jackahl
Copy link
Sponsor Member

@jackahl jackahl commented Sep 25, 2024

This PR is for issue #700. Most of the code is taken from #518, as it was easier to copy out the relevant code than to fix the merge conflicts.

The related issue in volto is plone/volto#6302

@mister-roboto
Copy link

@jackahl thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

Copy link
Sponsor Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

Couple minor comments, but basically looks good to me.

plone/app/contenttypes/schema/image.xml Outdated Show resolved Hide resolved
</description>
<required>False</required>
<title i18n:translate="label_alt_text">Alt Text</title>
</field>
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Should the field go after the image field? I've always put it there when adding an alt text field for specific projects.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for moving it after the image field

@davisagli
Copy link
Sponsor Member

Uh, there are failing tests though

@jensens
Copy link
Sponsor Member

jensens commented Sep 26, 2024

I like this long needed addition - but: this needs more work to get it into all places where tags are generated, esp this are the scales. Merging this without consistent output also in Classic UI does not help.

@jackahl
Copy link
Sponsor Member Author

jackahl commented Sep 26, 2024

Uh, there are failing tests though

trying to fix them, but I am very unfamiliar with Classic Plone templates (I think those should be the cause for the test failures), would appreciate some help from someone at the @plone/classicui-team here then

@jackahl
Copy link
Sponsor Member Author

jackahl commented Sep 26, 2024

I like this long needed addition - but: this needs more work to get it into all places where tags are generated, esp this are the scales. Merging this without consistent output also in Classic UI does not help.

Agreed. Though I'd say this should be better done and evaluated by the classic UI team. Would someone at the @plone/classicui-team be willing to champion this effort?

@petschki
Copy link
Member

petschki commented Sep 26, 2024

Since most of the images are created with the @@images view this needs to be implemented here too https://github.com/plone/plone.namedfile/blob/master/plone/namedfile/scaling.py#L158

Another place is the resolve_uid_and_caption transform which also tries to set the alt parameter, but I could not quickly find where its defined.

And there are maybe some more places wich have to be updated ... pandoras box has opened 😉

@MrTango
Copy link
Contributor

MrTango commented Sep 26, 2024

resolve_uid_and_caption

used to do it, but this is wrong, as images inside of TinyMCE should get the correct ALT text by the Editor.
This generated ALT text should only be used in listings, not everywhere where images are used. Only the Editor can know if an ALT text is needed and what it has to contain. Sometime an empty one is what you need.

@yurj
Copy link
Contributor

yurj commented Sep 27, 2024

The value of alt_text should be the default of the input when adding the image. Then the editor can choose to keep it or change or remove it. Win win, very few changes all backward compatible.

Also @davisagli suggested to implement it as a behaviour, so this would happen only when the behaviour is applied.

@jackahl
Copy link
Sponsor Member Author

jackahl commented Sep 27, 2024

The value of alt_text should be the default of the input when adding the image. Then the editor can choose to keep it or change or remove it. Win win, very few changes all backward compatible.

Also @davisagli suggested to implement it as a behaviour, so this would happen only when the behaviour is applied.

@yurj seems like a decent idea as well. I am quite agnostic about whats the best solution. But I think gathering some more opinions might help, coming to a decision.

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

Successfully merging this pull request may close these issues.

8 participants