-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Cover Block: Allow alt text #33226
Cover Block: Allow alt text #33226
Conversation
Size Change: +158 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
@kjellr, I think PR is ready for testing. I need to update fixtures for the integration test, but this isn't a blocker for manul tests. |
I've regenerated fixtures using this guide. After manual review, I think regenerated ones are correct. |
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.
This works great in my testing. I tried the sample text from the original issue, and also tried adding a few images and manually adding alt text using the new field. The field properly disappears when the block has no image, or when it is using video as media instead. Nice work!
I'll let someone else review the code, but functionally this seems good to go.
Hi, @ntsekouras I would love to get your opinion if this PR needs deprecation. We already had the |
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.
Thanks for working on this George! 👍
Code looks good - just rebase to resolve conflicts.
We don't need a deprecation in this case.
380775e
to
66db0d1
Compare
Thanks for having a look, Nik. Rebased and resolved conflicts. Waiting for checks to pass 🤞 |
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.
Thanks! 👍
@Mamaduka --Do you know if there is there a timeline for when this will be merged into a core release? |
Hey @allisonplus - since this is an enhancement, it will be included in |
Just wanted to drop in and say thanks to everyone for raising and fixing this! :) Waiting for 5.9 now. |
Description
Adds setting to provide alt text when image element is used for a Cover block.
I don't think we need deprecation for this feature since we already had an empty alt text attribute for image elements.
Fixes #30505.
How has this been tested?
Screenshots
Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).