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 in editing context #11218

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@antpb

antpb commented Oct 29, 2018

NEW PR TO CLEAN UP REBASE

Description

Sets default alt values to fix #1520
Previous conversation can be found here: #10482

How to test:
create image block
add image with no alt value
inspect image to see that alt value is now set as a readable string "This image has an empty alt attribute, its file name is "FILENAME""

@catehstn

This comment has been minimized.

catehstn commented Nov 1, 2018

@noisysocks @talldan Can you please have a look at this? It's a relatively minor change and would be good to get it merged. Thank you!

@talldan

Hi @antpb - thanks so much for working on this, looks like a really good change to get in, and the functionality worked well in testing.

I think there are a couple of code quality improvements that could be made, so I've left a some comments.

I can also help out adding an e2e test once this is merged.

// Disable reason: Image itself is not meant to be
// interactive, but should direct focus to block
// eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions
const imgNoAlt = <img src={ url } alt={ defaultAlt } onClick={ this.onImageClick } />;

This comment has been minimized.

@talldan

talldan Nov 2, 2018

Contributor

I think this could have been done in a much smaller change, something along the lines of:

const defaultedAlt = alt ? alt : sprintf( __( 'This image has an empty alt attribute; its file name is %s' ), this.getFilename( url ) );
const img = <img src={ url } alt={ defaultedAlt } onClick={ this.onImageClick } />;

Would avoid the need for the if statement on line 486 and the ternary on 574.

This comment has been minimized.

@antpb

antpb Nov 5, 2018

great suggestion. that makes this much more readable.

@@ -245,6 +246,16 @@ class ImageEdit extends Component {
};
}
getFilename( url ) {
if ( url ) {
const fileName = url.toString().match( /.*\/(.+?)$/ );

This comment has been minimized.

@talldan

talldan Nov 2, 2018

Contributor

It looks like the regular expression doesn't take query string parameters into account, and I think it should strip them.

For example, the image for my gh avatar has some params in the url -> https://avatars2.githubusercontent.com/u/677833?s=60&v=4

I also think the toString might be unnecessary, I'd expect url to be a string or undefined given that's its attribute type.

This comment has been minimized.

@antpb

antpb Nov 5, 2018

Thanks! Nice callout. My regex is horrible so I would love a review on my recent commit. I tested with a regular media library image and also the avatar string you provided. Seems to be working! 😬

This comment has been minimized.

@talldan

talldan Nov 6, 2018

Contributor

One change I'd make is to for the second group to be a non-capturing group, since we're not concerned with using its matched content:

/.*\/(.+?)(?:\?.*)?$/

This comment has been minimized.

@antpb

antpb Nov 6, 2018

Cool! Good call. Thanks!

Show resolved Hide resolved packages/block-library/src/image/edit.js Outdated
@@ -525,7 +537,6 @@ class ImageEdit extends Component {
showRightHandle = true;
}
}
/* eslint-enable no-lonely-if */

This comment has been minimized.

@aduth

aduth Nov 6, 2018

Member

This shouldn't be changed. We need to reenable any ESLint immediately after it is no longer necessary to be disabled.

return fileName[ 1 ];
}
}
return '';

This comment has been minimized.

@aduth

aduth Nov 6, 2018

Member

Won't this produce a nonsense alt value of 'This image has an empty alt attribute; its file name is ' if this return is reached?

This comment has been minimized.

@antpb

antpb Nov 6, 2018

@aduth Thanks! I do not believe there is an instance that url will not pass given the Media Library is where this is passed back from. I can't imagine how it would not return url. I only added this check as an edge case precaution of a graceful return instead of an error of using an undefined url. Can you think of any edge cases where url would return false? Happy to remove the check; I just want to make sure that I'm not taking away a safety net. :)

This comment has been minimized.

@aduth

aduth Nov 6, 2018

Member

Well, ideally we're certain the sort of data we're expecting. For something like URL user input, it's a bit harder to nail down. But maybe it's safe to assume that every URL should at least have a / followed by a "filename" (this is what the pattern is checking, yeah? The fact I'm asking is maybe a hint it's not very obvious on its own). If we are going to be uncertain, we should at least follow-through on our uncertainty, where defaultedAlt doesn't perform the substitution at all if the URL can't be determined.

@@ -246,6 +247,11 @@ class ImageEdit extends Component {
};
}
getFilename( url ) {
const fileName = url.match( /.*\/(.+?)(?:\?.*)?$/ );

This comment has been minimized.

@aduth

aduth Nov 12, 2018

Member

The main reason I'd see to author this way vs. directly return the value (return url.match( /* ... */ )[ 1 ]) would be to have the variable serve as an explaining variable, but in this case its not well-explained in that the return of String#match is not the file name, it's a match Array.

This comment has been minimized.

@aduth

aduth Nov 12, 2018

Member

This could be made more durable (especially the query parameter) bit if we were to rely on the new getPath from @wordpress/url.

const path = getPath( url );
if ( path ) {
	return last( path.split( '/' ) );
}

This comment has been minimized.

@aduth

aduth Nov 12, 2018

Member

To our previous conversation about the certainty of the file name being matched; is there any guarantee that the input here is in-fact a url? It's making me a bit nervous.

Show resolved Hide resolved packages/block-library/src/image/edit.js Outdated

antpb added some commits Nov 12, 2018

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