-
Notifications
You must be signed in to change notification settings - Fork 278
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(plugin): add aria-* visualizer plugin #142
base: master
Are you sure you want to change the base?
Conversation
CLA signature looks good 👍 |
I haven't forgotten this. I will try to make time this week to give feedback. Thank you for your patience. |
this.ariaAttributes = ATTRIBUTES.reduce((functionMap, attribute) => { | ||
const methodSuffix = formatAttributeForMethodName(attribute); | ||
|
||
functionMap[attribute] = { |
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 probably a much better way to handle this, but I wasn't sure how we'd deal with all the various different approaches you might need to take to visualizing the semantics of some of the aria-*
properties.
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.
Yeah, I'm not certain yet how we'd do that either. Might be something that evolves a bit as we add support for different aria-*
features.
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.
This looks like a reasonable approach so far.
I think having it show the roles like this and then also hide things that are aria-hidden
(harder to do, but it would be great if we could somehow show things that a screen reader would see but we don't visually - perhaps you already planned for that).
plugins/aria-visualizer/index.js
Outdated
|
||
let Plugin = require("../base"); | ||
|
||
let annotate = require("../shared/annotate")("roles"); |
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.
Could you use const
instead of let
?
@@ -0,0 +1,104 @@ | |||
/** | |||
* Allows users to see what screen readers would see. | |||
*/ |
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.
Thanks for the comment!
this.ariaAttributes = ATTRIBUTES.reduce((functionMap, attribute) => { | ||
const methodSuffix = formatAttributeForMethodName(attribute); | ||
|
||
functionMap[attribute] = { |
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.
Yeah, I'm not certain yet how we'd do that either. Might be something that evolves a bit as we add support for different aria-*
features.
plugins/aria-visualizer/index.js
Outdated
} | ||
|
||
getTitle() { | ||
return "aria-* Visualizer"; |
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 wonder if we could call this something a little more descriptive?
Spitballing:
- "Render as Screen Reader"
- "Screen Reader View"
- "ARIA View"?
Not super important; just thinking aloud.
plugins/aria-visualizer/index.js
Outdated
} | ||
|
||
getDescription() { | ||
return "See the effects of your aria-* and other a11y attributes"; |
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.
What do you think about making the description speak more to why one would want to use this feature rather than what id does? Perhaps something like, "View the page as though you were the screen reader. What things are visible, what aren't; what text will be read out, etc."
<p>This paragraph is visible.</p> | ||
<p id="p-with-hidden">This paragraph has a <span id="span" aria-hidden="true">hidden</span> span.</p> | ||
<p id="hidden-p" aria-hidden="true">This paragraph is hidden.</p> | ||
</article> |
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 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.
That was an error on my part. Should be working as expected after the next push
.
6a08702
to
a7a728a
Compare
@somewhatabstract I haven't quite figured out how to approach other attributes for the visualization side of things. It might be good to take some time to compile a list of different attributes we want to tackle, prioritize them, and then muse on what a good visualization for that might be. I think #128 would be a good place for that discussion. I'm going to leave this one open as a draft so I can continue to rebase changes and implement new visualizations as we come up with them. |
b3a2c38
to
682b83c
Compare
682b83c
to
968f87b
Compare
This is a first pass at working on the aria- Visualizer* plugin.
It currently only supports
aria-hidden
, but will be made to work with morearia*
attributes and be made more robust in general in the near future.It leverages some of the info panel updates from #139 and adds a new option to disable the annotation checkbox for plugins that don't have annotation.
I feel like I probably need to add annotation for both the plugin and the Focus Tracker added in #139, but I don't know enough about how it's implemented in this codebase yet. Regardless, I think it's a nice option to have for situations where we're using the info panel as more of a status indicator.
Closes #128