Menubar Navigation Example: Make single page and improve implementation of APG coding practices #1612
Conversation
Regression test coverage:Examples without any regression tests:
Examples missing some regression tests:
Example pages with Keyboard or Attribute table rows that do not have data-test-ids:
SUMMARY:52 example pages found. ERROR - missing tests: Please write missing tests for this report to pass. |
| this.fillerTextSentences.push( | ||
| 'The content on this page is associated with the <a href="$linkURL">$linkName</a> link for <a href="$siteURL">$siteName</a>.' | ||
| ); | ||
| // this.fillerTextSentences.push('The text content in this paragraph is filler text providing a detectable change of content when the <a href="$linkURL">$linkName</a> link is selected from the menu. '); |
| } | ||
| ); | ||
|
|
||
| /* |
There was a problem hiding this comment.
This comments out most 90% of the tests
There was a problem hiding this comment.
delete the commented code
There was a problem hiding this comment.
Think this comment was for the other line
mcking65
changed the title
|
@jongund I'll work on cautionary text for the intro. Could you please update the HCM documentation to use the language we just put in menubutton? |
|
@mcking65 |
| @@ -0,0 +1,4 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
I think you can delete this file, now that you have moved the svg definition into the css! :)
test/util/queryElements.js
Outdated
| * | ||
| * @returns {Promise} Resolves to array of elements | ||
| */ | ||
| module.exports = async function queryElements(t, selector, context) { | ||
| module.exports = async function queryElements(t, selector, context, noTest) { |
There was a problem hiding this comment.
I think we should probably add an "options" object with { allowNoResults : true }. The pattern you used is called a "boolean trap" and it's very hard to understand when reading the code.
Just a heads up: We also created this function queryElements to make sure that we don't have false positives in tests, especially when we are testing in a loop. If we are looping over elements returned by queryElements to test each element, but the css selector was incorrect, then we never enter the loop and the test false passes!
There was a problem hiding this comment.
@spectranaut
I don't need the change anymore, no need for no result option.
test/util/assertAriaOwns.js
Outdated
| * @param {String} elementSelector - the element with aria-owns set | ||
| */ | ||
|
|
||
| module.exports = async function assertAriaOwns(t, elementSelector) { |
There was a problem hiding this comment.
I appreciate this addition to the helper functions! But I noticed you don't actually use it in your test...? You could use it for the menubar-menuitem-aria-owns test
| const assertAriaRoles = require('../util/assertAriaRoles'); | ||
| const assertRovingTabindex = require('../util/assertRovingTabindex'); | ||
| const assertAriaOwns = require('../util/assertAriaOwns'); |
There was a problem hiding this comment.
Looks like you don't use this?
|
@nschonni |
|
@jongund it looks like the examples index is ok here, except that there are git conflicts in a few files |
|
This pull request is now part of pull #1741. |
|
@jongund I assume it is OK to delete this branch? |
|
@mcking65 |
|
@jongund yes, it is helpful to delete branches you create and are no longer needed. |
Update to menubar navigation example is ready for review:
Preview Link
Changes:
Review checklist