Skip to content
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

Merged
merged 4 commits into from
Aug 23, 2018
Merged

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented Aug 15, 2018

Do not require media captions / descriptions
Closes #816

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: @JKODU

@WilcoFiers WilcoFiers requested a review from a team as a code owner August 15, 2018 12:25
@@ -27,6 +27,27 @@ function hasUniqueId() {
};
}

function hasTwoOutcomes(messages) {
Copy link
Contributor

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?

}
// propferties: {
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, delete this comment

Copy link
Contributor

@jeeyyy jeeyyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

Copy link
Contributor

@dylanb dylanb left a 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?

}
// propferties: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, delete this comment

@@ -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"
Copy link
Contributor

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

@WilcoFiers
Copy link
Contributor Author

@dylanb @JKODU Updated the PR.

@WilcoFiers WilcoFiers merged commit 289f623 into develop Aug 23, 2018
@WilcoFiers WilcoFiers deleted the media-no-fails branch August 23, 2018 14:51
@WilcoFiers WilcoFiers mentioned this pull request Aug 27, 2018
8 tasks
WilcoFiers added a commit that referenced this pull request Aug 27, 2018
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants