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

Add aria practice guide links to reference.csv(s) #351

Merged
merged 2 commits into from
Dec 3, 2020

Conversation

evmiguel
Copy link
Contributor

@evmiguel evmiguel commented Dec 2, 2020

@evmiguel evmiguel requested a review from rmeritz December 2, 2020 21:43
@jscholes
Copy link
Contributor

jscholes commented Dec 2, 2020

@evmiguel I'm not sure I understand the intent behind/utility of this. Using the checkbox tests as an example, there is an "example" row with the URL: https://w3c.github.io/aria-practices/#checkbox. But this is wrong; that URL is the link to the design pattern in the APG, not the example being tested. Therefore, during development of new pattern tests, we've been using that row for the actual example link, the one that in this PR is called "practiceGuide".

In other words:

  1. These seem back-to-front. If we need rows for the APG section and example, they should be reversed.
  2. Do we need both? The example will always link to the APG section.
  3. If we need both, maybe "pattern", "designPattern" or similar is clearer than "practiceGuide"?

Let me know your thoughts.

@evmiguel
Copy link
Contributor Author

evmiguel commented Dec 2, 2020

@jscholes Thank you for taking a look at this. We need both the example value and what I have called practiceGuide for ARIA-AT app, as we want to link to both the example and the documentation around the pattern. Given your feedback, I will update the key to designPattern.

@rmeritz
Copy link
Contributor

rmeritz commented Dec 2, 2020

@evmiguel - This looks good can you update the "How to write tests" to add this as a refId before merging?

Also, @jscholes wanted to make sure you are aware that we are adding another field to the list of fields that should be added the the references.csv. It is called practiceGuide it links to the high-level design pattern for an example. Like https://www.w3.org/TR/wai-aria-practices-1.1/#checkbox. (This URL is already referenced in most refernce.csv files but not in a way that makes it clear across examples what is the most relevant high-level link is.

This change is being made to enable us to grab that data and display it in a new aria-at-app reports page.

@jscholes
Copy link
Contributor

jscholes commented Dec 2, 2020

@rmeritz

It is called practiceGuide it links to the high-level design pattern for an example. Like https://www.w3.org/TR/wai-aria-practices-1.1/#checkbox.

Understood, thanks for the additional context. The changes in tests/checkbox/data/references.csv are the wrong way round. In the updated file, "designPattern" links to the example while "example" links to the pattern.

@evmiguel
Copy link
Contributor Author

evmiguel commented Dec 2, 2020

@rmeritz

It is called practiceGuide it links to the high-level design pattern for an example. Like https://www.w3.org/TR/wai-aria-practices-1.1/#checkbox.

Understood, thanks for the additional context. The changes in tests/checkbox/data/references.csv are the wrong way round. In the updated file, "designPattern" links to the example while "example" links to the pattern.

@jscholes I noticed that, and I changed it!

@@ -1,7 +1,8 @@
refId,value
author,Michael Fairchild
authorEmail,[email protected]
reference,reference/2020-11-23_175030/checkbox-1/checkbox-1.html
Copy link
Contributor

Choose a reason for hiding this comment

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

@evmiguel - There is an actual problem you seem to have lost my changes because I think you need to rebase. This needs to not change.

@@ -1,8 +1,9 @@
refId,value
author,Jon Gunderson
authorEmail, [email protected]
reference,reference/2020-11-23_175528/menubar-editor.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Same problem.

@jscholes
Copy link
Contributor

jscholes commented Dec 2, 2020

Also, in tests/combobox-autocomplete-both/data/references.csv, "authorEmail" is mistyped as "authofEmail". Not sure if that's in scope to fix here, given that:

  1. it presumably wasn't introduced in this PR; and
  2. these tests will most likely be replaced with an updated version anyway.

@evmiguel evmiguel force-pushed the aria-practice-link-reference-csv branch 4 times, most recently from bf5d44e to 0ca37d6 Compare December 2, 2020 23:40
@evmiguel evmiguel force-pushed the aria-practice-link-reference-csv branch from 0ca37d6 to 52b50fd Compare December 2, 2020 23:49
@evmiguel evmiguel merged commit 4398954 into master Dec 3, 2020
@evmiguel evmiguel deleted the aria-practice-link-reference-csv branch December 3, 2020 00:02
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.

3 participants