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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Nov 12, 2018
Merged

adds default alt values in editing context #11218

Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
338eb64
Squashing commits - default alt values editor
antpb Nov 5, 2018
a693e89
Merge branch 'master' into add-default-alt-values-to-images-in-editin…
antpb Nov 5, 2018
b1327b8
adds suggestions by @talldan review - THANKS!
antpb Nov 5, 2018
632d820
makes second group non-capturing
antpb Nov 6, 2018
918e0e6
adds back the re-enable of no-lonely-if
antpb Nov 6, 2018
d93e67e
removes url check as it will never be false when this function is used
antpb Nov 9, 2018
b04f8d4
Merge branch 'master' into add-default-alt-values-to-images-in-editin…
antpb Nov 12, 2018
f7798f4
changes defaultAlt logic
antpb Nov 12, 2018
4e81635
Merge branch 'add-default-alt-values-to-images-in-editing-context-#15…
antpb Nov 12, 2018
4582845
makes use of getPath and last to determine the file name
antpb Nov 12, 2018
b90b62c
removes extra function from merge conflict
antpb Nov 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions packages/block-library/src/image/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { __, sprintf } from '@wordpress/i18n';
import { Component, Fragment } from '@wordpress/element';
import { getBlobByURL, revokeBlobURL, isBlobURL } from '@wordpress/blob';
import {
Expand Down Expand Up @@ -99,6 +99,7 @@ class ImageEdit extends Component {
this.updateDimensions = this.updateDimensions.bind( this );
this.onSetCustomHref = this.onSetCustomHref.bind( this );
this.onSetLinkDestination = this.onSetLinkDestination.bind( this );
this.getFilename = this.getFilename.bind( this );
this.toggleIsEditing = this.toggleIsEditing.bind( this );

this.state = {
Expand Down Expand Up @@ -246,6 +247,16 @@ class ImageEdit extends Component {
};
}

getFilename( url ) {
if ( url ) {
const fileName = url.match( /.*\/(.+?)(\?.*)?$/ );
if ( fileName ) {
return fileName[ 1 ];
}
}
return '';
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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. :)

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

getImageSizes() {
antpb marked this conversation as resolved.
Show resolved Hide resolved
return get( this.props.image, [ 'media_details', 'sizes' ], {} );
}
Expand Down Expand Up @@ -475,10 +486,11 @@ class ImageEdit extends Component {
imageHeight,
} = sizes;

const defaultedAlt = alt ? alt : sprintf( __( 'This image has an empty alt attribute; its file name is %s' ), this.getFilename( url ) );
antpb marked this conversation as resolved.
Show resolved Hide resolved
// 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 img = <img src={ url } alt={ alt } onClick={ this.onImageClick } />;
const img = <img src={ url } alt={ defaultedAlt } onClick={ this.onImageClick } />;

if ( ! isResizable || ! imageWidthWithinContainer ) {
return (
Expand Down Expand Up @@ -525,7 +537,6 @@ class ImageEdit extends Component {
showRightHandle = true;
}
}
/* eslint-enable no-lonely-if */
Copy link
Member

Choose a reason for hiding this comment

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

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


return (
<Fragment>
Expand Down