Closed Dieterrr closed 5 years ago
Please add snapshot tests for the new components.
Worth noting all the similar cards on MyYoast and yoast.com have already been (ot they're going to be) changed to reduce the amount of links pointing to the same resource. It would be great to follow that pattern and combine the image and title in one single link. See:
https://github.com/Yoast/my-yoast/pull/2181 https://github.com/Yoast/yoast.com/pull/3911 https://github.com/Yoast/yoast.com/pull/3908 https://github.com/Yoast/yoast.com/pull/3859
@afercia Just saw your comment. Tnx. I'll wait for the full PR now first to move this issue ahead. You are right about the problem: The card has 3 links to the same destination (I did not know this was an accessibility issue). However, I do not immediately know how to solve this as all 3 links are nested in different components. Any suggestions are welcome :)
@Dieterrr yep I know. It's one of the dangers of componentization ¯_(ツ)_/¯
A bit similar to https://github.com/Yoast/my-yoast/pull/2181/files where I had to move the title from CourseDetails.js
to Card.js
. Moving the title seemed the easier way to me.
@afercia About reducing the amount of links. @Xyfi had the smart observation that the header does not add any value for screenreaders so we might as well hide it. It is basically just an image with a link in it. If we do that we're only left with two links: one in the title and one in the "Read more about his course". What do you think of this solution?
@Dieterrr @Xyfi hiding it with aria-hidden
? That wouldn't help so much because the image link would still be focusable. Additionally, it would be an "empty" (meaning there wouldn't be anything to announce) tab stop. Also tabindex="-1"
wouldn't help so much because with screen readers you can still get to the image link. I'd tend to think having these cards consistent with the other card implementations would be a better option.
@afercia Ok, clear, tnx, I implemented your suggestion and moved the title to the header like in the other PRs.
CR 🚧 Please fix undefined
s in snapshots
CR 👍
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
This PR can be tested by following these steps:
Fixes #1877