Issue752 add index utility #951

jongund · 2018-12-06T17:26:32Z

This PR addresses issue #752.

The reference-tables.js is a node utility in the scripts directory that generates an index.html file in the examples directory. The index.html file is very simple with two tables and some navigation links to the practices document.

The appendices section of the aria-practices.html includes a new subsection for index with links to the indexes in examples/index.html.

View the new indexes section in this feature branch.

Preview | Diff

jongund · 2018-12-06T17:31:33Z

It seems to pass all the js linter tests, but required me to user older style Javascript, so while the coding change recommendations worked to generate the index.html file, they would not pass the js linter we are currently using, so I had to modify them to conform to the linter.

mcking65 · 2018-12-07T03:41:32Z

@nschonni, could you please review this? I tried requesting a review, but you do not show in the list. It looks like you are not a member of the aria contributors team in github.

nschonni

Mostly minor style stuff. For the node scripts using let and const would probably be a good idea

nschonni · 2018-12-07T04:00:54Z

scripts/reference-tables.js

+var examplesDirectory = '../examples/';
+
+var ariaRoles = [
+  'banner',


minor, but I'd suggest sorting the array so it's easier to see if anything is missing or when adding another one

I see duplication. Could we generate the list from the ARIA spec? We could use ARIA query for this.

nschonni · 2018-12-07T04:11:26Z

scripts/reference-tables.js

@@ -0,0 +1,409 @@
+var fs = require('fs');
+var i;


Seems like you mostly use local copies for loops. Maybe drop this and use let

nschonni · 2018-12-07T04:14:41Z

scripts/reference-tables.js

+    indexStart = content.indexOf('>', indexStart) + 1;
+    indexEnd = indexStart + 1;
+
+    console.log('Replacing at: ' + indexStart + ' .... ' + indexEnd);


aside, since this is for node, you can probably safely use template literals https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

console.log(`Replacing at: ${indexStart} .... ${indexEnd}`);

nschonni · 2018-12-07T04:19:50Z

scripts/reference-tables.js

+  }
+}
+
+var count = 0;


Think this should be inside the findHTMLFiles function

nschonni · 2018-12-07T04:21:24Z

scripts/reference-tables.js

+
+function findHTMLFiles (path) {
+  fs.readdirSync(path).forEach(function (file) {
+    var newPath = path + '/' + file;


probably safer for multiplatform to use the node path module and use path.join(filePath, file)

nschonni · 2018-12-07T04:22:28Z

scripts/reference-tables.js

+
+var count = 0;
+
+function findHTMLFiles (path) {


Might be better to use the glob pack to just filter the files you want

nschonni · 2018-12-07T04:23:24Z

scripts/reference-tables.js

+
+  // Handle landmark examples separately
+  if (res[r].indexOf('landmark') < 0) {
+  }


There is nothing in this condition, so right now this loop isn't doing anything

nschonni · 2018-12-07T04:26:21Z

scripts/reference-tables.js

+
+exampleIndexFile = replaceSection('examples_by_role_tbody', exampleIndexFile, html);
+
+sorted = [];


probably clearer to use a new variable than re-using the existing one

The linter we use flas let and const as errors

OK, there should probably be some overrides for the scripts in this folder since they're node rather than browser based. That can be done afterwards though

mcking65 · 2018-12-07T05:23:42Z

@jongund,

It seems some roles are being left out of the index. For instance, radiogroup is in the role index, but radio is not. Similarly, tablist is included, but tab is not.

I pushed b1bf298 to make two minor editorial changes to the template.

I tried to run the script to update the index with the changes, but it failed with an error related to an invalid call back.

jongund · 2018-12-07T15:12:08Z

@mcking65 I just get a deprecation error and it generates the index, so I will look it to getting the bug fixed. I think making it a Respec document is a little distracting, I liked the index as you originally proposed as a simple document that indexes the examples. The simpler document though should have a table of contents.

jongund · 2018-12-07T15:18:08Z

@mcking65 I will look into the problem of missing roles from the table.

jongund · 2018-12-07T15:34:45Z

@mcking65 sorry for the confusion about respec, I was looked at the wrong local directory on my computer, I am working on the role problem

jongund · 2018-12-07T16:08:31Z

@mcking65 I fixed the bug that was not including all the roles in the index, I am working on the issue of the the call back issue.

jongund · 2018-12-07T16:24:55Z

@mcking65 I fixed the bug related to the callback error, let me know if you can run the script now

jongund · 2018-12-07T16:35:08Z

@mcking65 I added two links at the beginning of the document to the two types of indexes on the page. I think it makes it easier for people to know there is an index of properties and states without having to scroll through the document.

* Simplified labels on nav regions because the nav regions at top and bottom are identical and have identical purpose, so they do not need unique labels. * Added a 1-sentence intro paragraph to make it more clear that this page is part of the APG. This would be important if google landed someone on this index page.

mcking65

@jongund,

We are getting very close here. I fixed lint errors, made a couple more changes to the template, and integrated into aria-practices.html in the appendices.

Please:

Reply to my questions about changes in package.json
In scripts/reference-tables.js, sort ariaRoles or consider my suggestion about aria.query.
In scripts/reference-tables.js, sort ariaPropertiesAndStates or consider my suggestion about aria.query.

mcking65 · 2018-12-12T14:42:06Z

package.json

@@ -28,8 +28,8 @@
    "ava": "^0.25.0",
    "cheerio": "^1.0.0-rc.2",
    "eslint": "^4.19.1",
-    "geckodriver": "^1.11.0",
+    "geckodriver": "^1.14.1",


@jongund, was this change necessary for this PR? It seems unrelated.

It should be thoroughly tested. I believe I recall @spectranaut finding geckodriver versions being a factor in performance of tests, race conditions, etc. It seems like we might want to isolate changes like this so they are easy to roll back given that our regression testing has geckodriver as a primary dependency.

I might be making noise about nothing. Perhaps @sh0ji, or @spectranaut if available, should chime in here.

I didn't modify this file, but I changed the values back

mcking65 · 2018-12-12T14:48:44Z

package.json

    "selenium-webdriver": "^4.0.0-alpha.1",
-    "vnu-jar": "^18.7.23"


Similar question here ... Is this change related to this PR?

@mcking65 I didn't directly modify this file, but I will change the value back

mcking65 · 2018-12-12T15:07:24Z

scripts/reference-tables.template

+  <script src="js/app.js"></script>
+</head>
+<body>
+  <nav aria-label="ARIA Practices">


I changed aria-label here and in the footer. It was previously "Top link to ARIA Practices". Per our landmark guidance, if two nav sections have identical content and identical purpose, they should have the same label, not distinct labels. Distinguishing regions with positional terms, "top" and "bottom" is not recommended.

jongund · 2018-12-19T18:24:37Z

@mcking65 The aria-query project maybe useful, but it provides more information than we need for the current index and it is not clear how files from the project should be merged into this pull request.

jongund · 2018-12-20T19:04:57Z

@mcking65 I have sorted the lists for roles, properties and state lists in the source code.

nschonni · 2018-12-20T19:36:19Z

scripts/reference-tables.js

+var ariaPropertiesAndStates = [
+  'aria-activedescendant',
+  'aria-atomic',
+  'aria-atomic',


Duplicate. There is a few more below, but I'll skip commenting them each individually

jongund · 2018-12-20T20:50:01Z

@mcking65 removed duplicate properties and states from list.

mcking65 · 2018-12-21T09:37:51Z

@nschonni, could you help us integrate this into travis build? I suppose it would go into travis-before_deploy.sh?

If you have time to spin up a PR, that would be great!

Add index utility (pull #951) For issue #752, adds a node script for generating two indexes: * Example by role * Examples by state or property The script lands the generated index.html in the /examples directory.

nschonni · 2018-12-22T20:24:24Z

@mcking65 do you want to exclude the file from the regular branch and just generate it as part of the gh-pages build?

jongund added 8 commits December 5, 2018 20:20

initial implementation of index of examples document

dac731e

initial implementation of index of examples document

98eb5b4

update file name and locations

eb8eb87

improved index codg by using reduce method

19f1d88

fixed bugs in lanmakr references

b6950d7

Merge branch 'master' into issue752-add-index-utility

8973428

fixed linting bugs due to lint testing for older versions of js

4ca81ba

fixed links to practices document

494a06a

fixed bug in property reference

d5aa4b4

mcking65 mentioned this pull request Dec 7, 2018

Issue 752 example index #947

Closed

mcking65 requested a review from jkva December 7, 2018 03:36

mcking65 self-assigned this Dec 7, 2018

nschonni reviewed Dec 7, 2018

View reviewed changes

reference-tables.template: Editorial revision to title tag and H1

b1bf298

fixed bug with some roles not being indexed

d708a1e

fixed call back bug

f3ca3ff

jongund added 2 commits December 7, 2018 10:26

updated index file of examples

3a4eb92

added links to sections at the top of the document

df7b590

mcking65 added 3 commits December 11, 2018 19:04

Merge branch 'master' into issue752-add-index-utility

30c15d4

Appendices Section: Add sub section for indexes

163d106

mcking65 added 3 commits December 12, 2018 00:01

Ran script to update indexes based on new template

a6453db

index.template: minor editorial revision

981899c

Autofix eslint errors

12e0daa

mcking65 requested changes Dec 12, 2018

View reviewed changes

mcking65 added this to the 1.1 APG Release 3 milestone Dec 12, 2018

mcking65 added enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy appendix labels Dec 12, 2018

jongund added 2 commits December 19, 2018 11:48

Merge branch 'master' into issue752-add-index-utility

2be2d95

restored values in package.json

b933379

jongund added 2 commits December 19, 2018 12:26

added copyright statement

52d7293

sorted the roles and prpoerty/states array

16d62df

update example index.html

4d0fa30

nschonni reviewed Dec 20, 2018

View reviewed changes

removed ruplicate properties and states from array

9813f62

mcking65 mentioned this pull request Dec 21, 2018

Develop basic carousel example #458

Closed

mcking65 approved these changes Dec 21, 2018

View reviewed changes

mcking65 merged commit 5ec6c8f into master Dec 21, 2018

mcking65 deleted the issue752-add-index-utility branch January 25, 2019 15:50

mcking65 mentioned this pull request Feb 10, 2019

Create utility to index the roles, properties and states used in examples #752

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue752 add index utility #951

Issue752 add index utility #951

jongund commented Dec 6, 2018 •

edited by pr-preview bot

Loading

jongund commented Dec 6, 2018

mcking65 commented Dec 7, 2018

nschonni left a comment

nschonni Dec 7, 2018

mcking65 Dec 7, 2018

nschonni Dec 7, 2018

nschonni Dec 7, 2018

nschonni Dec 7, 2018

nschonni Dec 7, 2018

nschonni Dec 7, 2018

nschonni Dec 7, 2018

nschonni Dec 7, 2018

jongund Dec 7, 2018

nschonni Dec 20, 2018

mcking65 commented Dec 7, 2018

jongund commented Dec 7, 2018

jongund commented Dec 7, 2018

jongund commented Dec 7, 2018

jongund commented Dec 7, 2018

jongund commented Dec 7, 2018

jongund commented Dec 7, 2018

mcking65 left a comment

mcking65 Dec 12, 2018

jongund Dec 19, 2018

mcking65 Dec 12, 2018

jongund Dec 19, 2018

mcking65 Dec 12, 2018

jongund commented Dec 19, 2018

jongund commented Dec 20, 2018

nschonni Dec 20, 2018

jongund commented Dec 20, 2018

mcking65 commented Dec 21, 2018

nschonni commented Dec 22, 2018


		exampleIndexFile = replaceSection('examples_by_role_tbody', exampleIndexFile, html);

		sorted = [];

		"selenium-webdriver": "^4.0.0-alpha.1",
		"vnu-jar": "^18.7.23"

Issue752 add index utility #951

Issue752 add index utility #951

Conversation

jongund commented Dec 6, 2018 • edited by pr-preview bot Loading

jongund commented Dec 6, 2018

mcking65 commented Dec 7, 2018

nschonni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcking65 commented Dec 7, 2018

jongund commented Dec 7, 2018

jongund commented Dec 7, 2018

jongund commented Dec 7, 2018

jongund commented Dec 7, 2018

jongund commented Dec 7, 2018

jongund commented Dec 7, 2018

mcking65 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongund commented Dec 19, 2018

jongund commented Dec 20, 2018

Choose a reason for hiding this comment

jongund commented Dec 20, 2018

mcking65 commented Dec 21, 2018

nschonni commented Dec 22, 2018

jongund commented Dec 6, 2018 •

edited by pr-preview bot

Loading