-
Notifications
You must be signed in to change notification settings - Fork 56
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
Cards: Use svg instead of text inside the placeholder image #1886
Conversation
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
site/layouts/shortcodes/card.html
Outdated
@@ -22,9 +22,11 @@ | |||
{{- $rtl := .Get "rtl" | default false -}} | |||
|
|||
<div class="card{{ if eq $borders false }} border-0{{ end }}{{ if and $borders (eq $background "dark") }} border-dark{{ end }}"> | |||
<svg class="bd-placeholder-img card-img-top" width="100%" height="162" xmlns="http://www.w3.org/2000/svg" role="img" aria-label="{{ if $rtl }}تعليق على الصورة{{ else }}Image cap{{ end }}" preserveAspectRatio="xMidYMid slice" focusable="false"> | |||
<svg class="bd-placeholder-img card-img-top" width="100%" height="169" xmlns="http://www.w3.org/2000/svg" role="img" aria-label="{{ if $rtl }}تعليق على الصورة{{ else }}Image cap{{ end }}" preserveAspectRatio="xMidYMid slice" focusable="false"> |
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.
Currently wondering if Image cap
is a good value for an aria-label
attribute?
It's outside the frame of the current PR so we may discuss it in a distinct issue.
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.
Seems to be on Bootstrap Side so yes, let's tackle it there directly!
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.
aria-hidden="true" focusable="false"
is good for a non-informative svg so ok from an accessibility point of view.
I am just wondering if we need to do the same modifications elsewhere?
@@ -20,7 +20,7 @@ Below is an example of a basic card with mixed content and a fixed width. Cards | |||
|
|||
{{< example >}} | |||
<div class="card" style="width: 18rem;"> | |||
{{< placeholder width="100%" height="180" class="card-img-top" text="Image cap" >}} | |||
{{< placeholder width="100%" height="180" class="card-img-top" text="icon" >}} |
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.
Was the transition from 'Image cap' to 'icon' validated by our a11y expert or do we need to ask him for a review?
Shouldn't it be tackled on Bootstrap's side? 🤔
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.
Added the right label for upcoming a11y review and yeah it might be proposed on their side as well.
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.
Ok for me, just one remaining question to check with a11y experts in our side and/or Bootstrap's side.
as discuss with @louismaximepiton, there's some improvement that has to be made in another PR, so ok on this one |
<rect width="100%" height="100%" fill="{{ (index $.Site.Data.grays 3).hex }}"/> | ||
<text x="50%" y="50%" fill="{{ (index $.Site.Data.grays 5).hex }}" dy=".3em">{{ if $rtl }}تعليق على الصورة{{ else }}Image cap{{ end }}</text> | ||
<svg x="30%" y="30%" width="40%" height="40%" viewBox="0 0 24 24" fill="#00000014" aria-hidden="true" focusable="false"> |
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.
We don't have a corresponding value in our $.Site.Data.grays
list?
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.
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.
None 😄 Should probably be a dark gray but nevermind, it's not that related to the PR. Edge case.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Note: Please transform
- [ ]
into- (NA)
in the description when things are not applicableRelated issues
Related to #1677.
Description
Use a svg instead of text.
Motivation & Context
Better a11y + might be better detected inside
pa11y-ci
.Closer to design.
Types of change
Live previews
Checklist
Contribution
Accessibility
Design
Development
Documentation
Checklist (for Core Team only)
After the merge