Improving Related Posts markup as a follow up to jetpack PR 11324 #31023
Conversation
| height="24" | ||
| viewBox="0 0 24 24" | ||
| > | ||
| <title>{ __( 'Icon for image' ) }</title> |
There was a problem hiding this comment.
I know this wasn't introduced in this PR, but noting that title is basically ignored by screen readers because SVG component hides the whole icon from screen readers with these props:
'aria-hidden': 'true',
focusable: 'false',
Not a blocker for this PR.
There was a problem hiding this comment.
This is why there is an aria-label in the figure :)
There was a problem hiding this comment.
Sure but isn't <title> still redundant? 🙃
<svg aria-hidden>
<title>...</title>
</svg>There was a problem hiding this comment.
Also, the Svg component in Gutenberg takes care of the aria-hidden and focusable stuff automatically.
There was a problem hiding this comment.
Yes, so that's what I meant to say that <title> isn't needed here at all, because SVG component adds aria-hidden automatically and thus screen readers will never actually read that title. Unless I'm missing something? :-)
There was a problem hiding this comment.
In summary, this seems redundant.
| <title>{ __( 'Icon for image' ) }</title> |
There was a problem hiding this comment.
True, but it annoys the linter.
There was a problem hiding this comment.
I'm confused, which linter/rule? 🙃
aldavigdis
force-pushed
the
related-posts-imrove-markup
branch
from
cb8d289 to
276d15f
Compare
| @@ -17,95 +17,92 @@ export const MAX_POSTS_TO_SHOW = 6; | |||
|
|
|||
| function PlaceholderPostEdit( props ) { | |||
| return ( | |||
| <div | |||
| <ul | |||
There was a problem hiding this comment.
Noting that since different themes introduce padding/margin to ul and li elements, would be good to make sure we reset them so not to have surprises.
There was a problem hiding this comment.
There is an !important statement in the Sass code that should cover this.
There was a problem hiding this comment.
Could we increase specifity in some other way than by using !important which is often a difficult pattern otherwise (https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity)? We just had a similar case with slideshow block #31019
Not a blocker for this PR tho!
| { __( 'Preview: Not enough related posts found' ) } | ||
| </strong> | ||
| <li className="jp-related-posts-i2__post-link"> | ||
| <a id={ props.id + '-heading' } href={ window.location }> |
There was a problem hiding this comment.
Would be good to guard against window not existing — this can happen e.g. in our testing setups.
Basically just;
typeof window !== 'undefined'
simison
added
the
[Status] Needs Review
|
Are you targetting this for 7.1? I was hesitant to add milestone if not. |
|
@simison — Yes — This need to make it in to 7.1. |
aldavigdis
force-pushed
the
related-posts-imrove-markup
branch
from
276d15f to
7a3fe93
Compare
simison
added
the
[Goal] Gutenberg
| className="jp-related-posts-i2__post-image-placeholder-figure-square" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| width="100%" | ||
| height="100%" |
There was a problem hiding this comment.
I don't believe 100% are valid values for width and height attributes. They should be integers representing a value in CSS pixels.
https://html.spec.whatwg.org/#dimension-attributes
Styling of this type should be handled by CSS.
| height="24" | ||
| viewBox="0 0 24 24" | ||
| > | ||
| <title>{ __( 'Icon for image' ) }</title> |
There was a problem hiding this comment.
In summary, this seems redundant.
| <title>{ __( 'Icon for image' ) }</title> |
| @@ -17,95 +17,95 @@ export const MAX_POSTS_TO_SHOW = 6; | |||
|
|
|||
| function PlaceholderPostEdit( props ) { | |||
There was a problem hiding this comment.
Whoa, there are 4 components defined in this file! I strongly prefer 1 component 1 file.
There was a problem hiding this comment.
They are all dependencies of a master component.
aldavigdis
force-pushed
the
related-posts-imrove-markup
branch
from
7a3fe93 to
7155cbe
Compare
matticbot
removed
the
[Status] Needs Review
Changes proposed in this Pull Request
has-small-font-sizeclasses from some HTML elements as this was causing a discrepancy between the editor (where this is defined) and the fronted, where is depends on the theme.Testing instructions
Test with various themes. Does anything break?