-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Try to improve the accessibility of featured images #7200
Changes from 5 commits
76f1946
6a40827
22288b1
a59112c
25ddc18
fa44d6c
e7712a1
4b0298d
253a530
20f5489
c01e844
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,10 +18,6 @@ import './style.scss'; | |
import PostFeaturedImageCheck from './check'; | ||
import MediaUpload from '../media-upload'; | ||
|
||
// Used when labels from post type were not yet loaded or when they are not present. | ||
const DEFAULT_SET_FEATURE_IMAGE_LABEL = __( 'Set featured image' ); | ||
const DEFAULT_REMOVE_FEATURE_IMAGE_LABEL = __( 'Remove featured image' ); | ||
|
||
function PostFeaturedImage( { featuredImageId, onUpdateImage, onRemoveImage, media, postType } ) { | ||
const postLabel = get( postType, [ 'labels' ], {} ); | ||
|
||
|
@@ -30,12 +26,12 @@ function PostFeaturedImage( { featuredImageId, onUpdateImage, onRemoveImage, med | |
<div className="editor-post-featured-image"> | ||
{ !! featuredImageId && | ||
<MediaUpload | ||
title={ postLabel.set_featured_image } | ||
title={ __( 'Set featured image' ) } | ||
onSelect={ onUpdateImage } | ||
type="image" | ||
modalClass="editor-post-featured-image__media-modal" | ||
render={ ( { open } ) => ( | ||
<Button className="editor-post-featured-image__preview" onClick={ open } isLink> | ||
<Button className="editor-post-featured-image__preview" onClick={ open }> | ||
{ media && | ||
<ResponsiveWrapper | ||
naturalWidth={ media.media_details.width } | ||
|
@@ -50,26 +46,36 @@ function PostFeaturedImage( { featuredImageId, onUpdateImage, onRemoveImage, med | |
/> | ||
} | ||
{ !! featuredImageId && media && ! media.isLoading && | ||
<p className="editor-post-featured-image__howto"> | ||
{ __( 'Click the image to edit or update' ) } | ||
</p> | ||
<MediaUpload | ||
title={ __( 'Set featured image' ) } | ||
onSelect={ onUpdateImage } | ||
type="image" | ||
modalClass="editor-post-featured-image__media-modal" | ||
render={ ( { open } ) => ( | ||
<Button onClick={ open } isDefault isLarge> | ||
{ __( 'Replace Image' ) } | ||
</Button> | ||
) } | ||
/> | ||
} | ||
{ ! featuredImageId && | ||
<MediaUpload | ||
title={ postLabel.set_featured_image || DEFAULT_SET_FEATURE_IMAGE_LABEL } | ||
onSelect={ onUpdateImage } | ||
type="image" | ||
modalClass="editor-post-featured-image__media-modal" | ||
render={ ( { open } ) => ( | ||
<Button className="editor-post-featured-image__toggle" onClick={ open } isLink> | ||
{ postLabel.set_featured_image || DEFAULT_SET_FEATURE_IMAGE_LABEL } | ||
</Button> | ||
) } | ||
/> | ||
<div> | ||
<MediaUpload | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like the exact same component/args/markup as above. Any chance we could refactor this a bit to reduce the duplication? |
||
title={ __( 'Set featured image' ) } | ||
onSelect={ onUpdateImage } | ||
type="image" | ||
modalClass="editor-post-featured-image__media-modal" | ||
render={ ( { open } ) => ( | ||
<Button className="editor-post-featured-image__toggle" onClick={ open }> | ||
{ __( 'Set featured image' ) } | ||
</Button> | ||
) } | ||
/> | ||
</div> | ||
} | ||
{ !! featuredImageId && | ||
<Button className="editor-post-featured-image__toggle" onClick={ onRemoveImage } isLink> | ||
{ postLabel.remove_featured_image || DEFAULT_REMOVE_FEATURE_IMAGE_LABEL } | ||
<Button onClick={ onRemoveImage } isDefault isLarge> | ||
{ __( 'Remove Image' ) } | ||
</Button> | ||
} | ||
</div> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,33 +1,38 @@ | ||
.editor-post-featured-image { | ||
padding: 10px 0 0; | ||
padding: 0; | ||
|
||
.spinner { | ||
margin: 0; | ||
} | ||
} | ||
|
||
.editor-post-featured-image__toggle { | ||
text-decoration: underline; | ||
color: theme( secondary ); | ||
|
||
&:focus { | ||
box-shadow: none; | ||
outline: none; | ||
// Space consecutive buttons evenly. | ||
.components-button + .components-button { | ||
margin-top: 1em; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eventually I'll propose sorting our CSS attributes alphabetically; until then I'll live with this 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :) I like your style. Though elaborate a bit — are you referring to the top margin going before the right margin? For north south east west things I always go clockwise, i.e. top, right, bottom, left. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know some folks do that (I used to as well), but yes I am. It makes sense, but I prefer consistency over sense. |
||
margin-right: 8px; | ||
} | ||
|
||
&:hover { | ||
color: theme( primary ); | ||
// Don't size up images beyond their intrinsic size. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "size up" is a bit odd, maybe: // This keeps images at their intrinsic size (eg. a 50px
// image will never be wider than 50px). |
||
.components-responsive-wrapper__content { | ||
max-width: 100%; | ||
width: auto; | ||
} | ||
} | ||
|
||
.editor-post-featured-image__toggle, | ||
.editor-post-featured-image__preview { | ||
display: block; | ||
width: 100%; | ||
padding: 0; | ||
} | ||
|
||
.editor-post-featured-image__howto { | ||
color: $dark-gray-300; | ||
font-style: italic; | ||
margin: 10px 0; | ||
} | ||
.editor-post-featured-image__toggle { | ||
border: 1px dashed $light-gray-900; | ||
background-color: $light-gray-300; | ||
line-height: 20px; | ||
padding: $item-spacing 0; | ||
text-align: center; | ||
|
||
&:hover { | ||
background-color: $light-gray-100; | ||
} | ||
} |
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.
Any reason to avoid the labels from the post object
postLabel.set_featured_image
andpostLabel.remove_featured_image
. These labels are tweakable server-side. We could use the proposed labels as a fallback if theset_featured_image
orremove_featured_image
are not defined?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've no strong opinion here. In this case I changed it so it was consistent with the other labels.
"Set featured image" is the only label we haven't customized. If you'd like, I can restore all instances where we use "Set featured image" as the label to use the postLabel again.
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 feel we should restore it essentially for Backwards Compatibility because CPT are probably customizing this label. And I feel we consistently use CPT's labels across the app. In which components are we avoiding those?
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 no moral objections to reapplying those labels. But here's what's in master:
Those labels match the ones from
postLabel
.If this branch gets merged sans changes, then we will have the following labels:
__( 'Set featured image' )
— this could match the postLabel__( 'Replace Image' )
— no match in postLabel__( 'Remove Image' )
— could match "Remove featured image", but it feels super redundant and would take up more space.The postLabels are definitely designed for the old interface:
If we can update the postLabel labels, that would be ideal, but I imagine that's not possible.
What do you think we should do?
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 personally don't think the labels for the featured images are that important because it's a generic thing:
replace_featured_image
label to the CPT labels. (Not certain how we define the existence of these labels)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.
Solid thoughts.
@pento, any idea here?
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.
@afercia you also have a treasure trove of core experience — do you have any insights here?
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.
@jasmussen sorry, I'm late to the party. Is your question related to how many plugins might use custom labels? Not a great plugins expert here, but I guess it's pretty common for plugins to customize them, just a couple examples: EDD and Woocommerce: