-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
adds default alt values for #1520 #10482
Conversation
|
||
if ( typeof caption !== 'object' ) { | ||
caption = create( { html: caption } ); | ||
} | ||
|
||
if ( ! alt ) { | ||
alt = 'This image has an empty alt attribute, the file name is ' + filename; |
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 message should be translatable
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.
NICE catch. Thank you.
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 approach works for me but as mentioned: the field should be translatable and it'd be great to add an E2E test for this (I guess in a11y
tests or in a new image
block test file).
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 needs to use sprintf()
for the translation to work.
Are you comfortable writing E2E tests for this as well? 😄
|
||
if ( typeof caption !== 'object' ) { | ||
caption = create( { html: caption } ); | ||
} | ||
|
||
if ( ! alt ) { | ||
alt = __( 'This image has an empty alt attribute, the file name is ' ) + filename; |
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 needs to use sprintf()
(example usage: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/heading/heading-toolbar.js#L18)
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.
Also, a bit pedantic but I think the comma should be a semi-colon, so it should be:
alt = sprintf( __( 'This image has an empty alt attribute; its file name is "%s"' ), filename );
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.
Cool. Changing. Does caption
also need this?
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.
Nope, caption
is not content that we provide text for; it doesn't needs to be localised.
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.
I have not worked on E2E test before, but that is no reason to avoid doing so now. If you have any resources I'd be super appreciative for them. I'll continue working through it today.
Sadly I’m not sure we have many references beyond the tests themselves and Google’s Puppeteer API docs. The docs are quite handy and the tests themselves are pretty readable because of Puppeteer’s quite friendly API. I’d recommended checking `test/e2e/specs/` for some tests to get a sense of how they work; you can always check in Slack’s #core-editor channel too for help :-)
|
I'm comfortable learning how to write the tests. I spent a bit of time reviewing the docs/source code of other tests. I think I can get this going. :) |
Worth noting here, in my learning to build tests, I found This line is where the alt value should not have passed: https://github.com/WordPress/gutenberg/blob/master/test/e2e/specs/adding-inline-tokens.test.js#L47
Alrighty! Latest PR fixes inline-image as well as image block. I'm still working on making the regular image tests. I've updated the inline tests to include the new default string. |
Nice! I'll have a look sometimes today, thanks for that! 👍 |
Expected:
Escaping in that string didnt seem to fix it (as you can see in the travis report.) |
Worth noting also the featured image would need the same information. The featured image preview uses a button with an aria-label, see also #10869 |
save( { id, url, alt, width } ) { | ||
save( { id, url, alt, width, filename } ) { | ||
if ( ! alt ) { | ||
alt = sprintf( __( 'This image has an empty alt attribute; its file name is \"%s\"' ), filename ); |
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.
No need for the escaping.
|
||
if ( typeof caption !== 'object' ) { | ||
caption = create( { html: caption } ); | ||
} | ||
|
||
if ( ! alt ) { | ||
alt = sprintf( __( 'This image has an empty alt attribute; its file name is \"%s\"' ), filename ); |
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.
No need for the escaping.
@@ -44,7 +44,7 @@ describe( 'adding inline tokens', () => { | |||
await page.click( '.media-modal button.media-button-select' ); | |||
|
|||
// Check the content. | |||
const regex = new RegExp( `<!-- wp:paragraph -->\\s*<p>a\\u00A0<img class="wp-image-\\d+" style="width:\\s*10px;?" src="[^"]+\\/${ filename }\\.png" alt=""\\/?><\\/p>\\s*<!-- \\/wp:paragraph -->` ); | |||
const regex = new RegExp( `<!-- wp:paragraph -->\\s*<p>a\\u00A0<img class="wp-image-\\d+" style="width:\\s*10px;?" src="[^"]+\\/${ filename }\\.png" alt="This image has an empty alt attribute; its file name is "10x10_e2e_test_image_z9T8jK.png""\\/?><\\/p>\\s*<!-- \\/wp:paragraph -->` ); |
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.
You have to use the HTML entity "
for the quotes to check for valid HTML.
Closing this as I have rebased and made all of the above code change recommendations at his PR: #10960. As for @afercia 's recent comment about featured image, I need to look into that. Might make a new issue so we can get these default alt values added asap. I have a PR already open that is manipulating featured image behavior. I'll see if I can just wrap the default alt values into there. |
Description
Sets default alt values to fix #1520 I am open to discussion around what the default value should be. I liked @afercia 's suggestion in the issue
How to test: