-
Notifications
You must be signed in to change notification settings - Fork 780
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
fix: Do not require media captions / descriptions #1075
Conversation
build/tasks/validate.js
Outdated
@@ -27,6 +27,27 @@ function hasUniqueId() { | |||
}; | |||
} | |||
|
|||
function hasTwoOutcomes(messages) { |
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.
Looks like this is normalising multiple outcomes, should change function name to something generic like normaliseOutcomes
or hasMultipleOutcomes
rather than exactly Two
?
build/tasks/validate.js
Outdated
} | ||
// propferties: { |
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.
minor: 🤣 propferties
, know it is commented.... delete instead?
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.
yeah, delete this comment
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.
See comments.
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.
Would be better if you removed the commented code.
Do you think it needs updates to the documentation?
build/tasks/validate.js
Outdated
} | ||
// propferties: { |
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.
yeah, delete this comment
lib/checks/media/caption.json
Outdated
@@ -5,7 +5,6 @@ | |||
"impact": "critical", | |||
"messages": { | |||
"pass": "The multimedia element has a captions track", | |||
"fail": "The multimedia element does not have a captions track", | |||
"incomplete": "A captions track for this element could not be found" |
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.
Could make this message more explicit about what to do to check whether an alternative exists
Review 10% (50 max) of the following (Product manager only): - [x] feat: Update FR (french) translation file for 3.1 release (#1089) (4a5cad0) - [x] chore: Fix example dependency mistake (#1094) (6af5733) - [x] fix: Do not require media captions / descriptions (#1075) (289f623) - [x] feat(aria-role): Remove non-existing "text" role (#1069) (67ec1f5) - [x] Merge pull request #1017 from dequelabs/aria-getrole (f64ff10) - [x] chore: remove `.jshintrc` files (#1028) (03b3327) - [x] chore: ability to add external libs (axios) (0957dab) - [x] Merge pull request #985 from dequelabs/git-update (54b0b60)
Do not require media captions / descriptions
Closes #816
Reviewer checks
Required fields, to be filled out by PR reviewer(s)