-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: Restructure APG Support Tables #1053
Conversation
So I think the design still looks pretty good, but I was wondering @isaacdurazo, given that this is by far the most public representation of the ARIA-AT projects work, if you have any ideas to spruce it up? One possible tweak might be "MUST" and "SHOULD" being all caps, in the past we've gotten accessibility notes about all caps words being hard for people with dyslexia to read. I do wonder if "Must Assertions" or something like that might be easier to understand? But the main thing motivating me to ask for your input is just the |
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.
Fantastic work, it's working perfectly. I was even able to test it integrated into a development version of the APG website. I do have a couple notes on the implementation itself. And I pinged Isaac in a comment to weigh in on the design given that this is arguably the most important page in the app.
server/apps/embed.js
Outdated
} | ||
} | ||
} | ||
} | ||
testPlanReports( |
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.
Is it possible to remove this part of the query?
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.
I don't think so. testPlanReports
is being used in the code.
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.
As I said in another comment, you're right that it's being used. My point is more that it's only being used by dead code and the whole network of dead code present in this file can be removed.
server/apps/embed.js
Outdated
@@ -233,10 +256,12 @@ const renderEmbed = ({ | |||
allAtBrowserCombinations, | |||
title: queryTitle || title || 'Pattern Not Found', | |||
pattern, | |||
priorities, | |||
phase, | |||
allBrowsers, |
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.
There's a lot of dead code still in this file, like this variable is not used anywhere for example. Would it be possible to simplify this file a bit? It's currently getting pretty intimidating. Acknowledging of course that I contributed more than anyone to the current situation.
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.
Same here. allBrowsers
is being used in the code.
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.
So yes the variable appears 7 times in this file, but all of those uses are only related to setting the variable, not using it. Then if you look at the helpers file and main.hbs you'll see that this variable is passed around a bit but never used. That's what I mean when I say it's dead code.
The larger issue, beyond this one variable, is that a lot of the code in this file is passed around and set but never actually utilized, and I think it's definitely worth the effort to overhaul the whole file and remove all the dead code.
I'm very sorry for my late response, @alflennik. In terms of look and feel I'm hesitant to move away from what we currently have because we need to follow how APG looks like. With regard to your suggestions for copy changes, I agree MUST and SHOULD should definitely not be all caps, and adding some extra context would be great. I've honestly always felt confused about the meaning of Must and Should and I'm not sure going with something like "Must Assesrtions" makes sense. When looking at a report in ARIA AT, like this one, these are under a column labeled as "Priority", so with that in mind should we maybe do something like "Priority: Must"? About the |
This pull request is ready for review. |
Latest submitted PR addresses w3c/aria-practices#3017 |
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.
Fantastic work!
Please don't merge this yet since we are still awaiting feedback from the APG task force.
…1096) Address #3018 Updates the design and color scheme of the expandable box dropdown to use a different base color (green) when the test plan is recommended.
The APG TF discussed this on May 21, 2024 and confirmed this work is now ready to be merged. I will be merging this later today. |
@howard-e |
@mcking65 this work being merged into the |
Issues addressed: * #1105, addresses w3c/aria-at#1070 * #1053, addresses w3c/aria-practices#2971 * #1097, addresses #977 * #1095, addresses #991 * #1093, addresses #934 * #1000, addresses #818 * #1089, addresses #992 * #1067, addresses #993 * #1056, addresses w3c/wai-aria-practices#212 --------- Co-authored-by: alflennik <[email protected]> Co-authored-by: Paul Clue <[email protected]> Co-authored-by: Mx Corey Frang <[email protected]> Co-authored-by: Mx. Corey Frang <[email protected]> Co-authored-by: Erika Miguel <[email protected]> Co-authored-by: Mike Pennisi <[email protected]>
see issue #w3c/aria-practices#2971
This pull request changes the structure of the support tables. The rows are now AT and browser combinations and the columns are now "MUST" and "SHOULD" assertions. The support percentage now represents the degree of support for "MUST" assertions and "SHOULD" assertions.