-
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 2 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' ], {} ); | ||
|
||
|
@@ -35,7 +31,7 @@ function PostFeaturedImage( { featuredImageId, onUpdateImage, onRemoveImage, med | |
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,47 @@ 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={ __( 'Replace 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 | ||
title={ __( 'No image selected' ) } | ||
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 title prop is the title of the modal I think, Are you certain you want this? Shoudl this also be |
||
onSelect={ onUpdateImage } | ||
type="image" | ||
modalClass="editor-post-featured-image__media-modal" | ||
render={ ( { open } ) => ( | ||
<Button className="editor-post-featured-image__toggle" onClick={ open }> | ||
{ __( 'No image selected' ) } | ||
</Button> | ||
) } | ||
/> | ||
<MediaUpload | ||
title={ __( 'Add Image' ) } | ||
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 think we should use the same title in the modal here as well "Set featured image'? |
||
onSelect={ onUpdateImage } | ||
type="image" | ||
modalClass="editor-post-featured-image__media-modal" | ||
render={ ( { open } ) => ( | ||
<Button onClick={ open } isDefault isLarge> | ||
{ __( 'Add 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 { | ||
display: block; | ||
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. |
||
} | ||
|
||
&: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.
This looks like the exact same component/args/markup as above. Any chance we could refactor this a bit to reduce the duplication?