Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(a11y): add data table for screen readers (sunburst, treemap, icicle, flame) #1155
feat(a11y): add data table for screen readers (sunburst, treemap, icicle, flame) #1155
Changes from 10 commits
ebbee47
d70b995
b7a030d
8340b51
863d6dc
547c7af
2acc45e
fcd3fc8
15132c7
51a8be3
b9b7dfd
7cb0d70
7e69878
d070a6c
cdef1c2
6f74271
99a6d61
a14cc62
060d4ff
77fce3a
8763b91
46cb755
df537b4
691b89d
e5f141e
d39ab17
4dd1136
e253fb3
71050c0
04edce2
aebfe90
e654f11
97b5b57
6c984bb
e1cf63b
554c3f2
12dd739
654668a
adb7bbe
fa0c61c
9fba9c1
204abef
793b81f
a65028a
ef443b0
17568c6
49165d0
dd1ba26
b180214
42b4f0e
3cd61db
be561f9
228bc32
c3a361d
d31fad8
5646771
3e5cd36
b0b02fc
82e8219
5dabaea
f929208
b019245
f98dee7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Wondering if 20 is good hardcoded, @markov00 might want to chime in. Also, even if it's too many, maybe the top N values could be kept, to not disadvantage the user with the loss of all items. Also, what's the harm in just doing it unconditionally for all sectors? Hopefully the VO user can "back out", ie. doesn't need to resort to hearing all the 0.1% slices before continuing with the rest of the page contents
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.
Should we really limit that, or is the user that can directly skip the content after the first top N values?
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 think the data table definitely impacts rendering performance when loading flame/icicle charts (particularly in terms of #1155 (comment)), so I wondering about implementing some sort of escape hatch if results are in the 1000s. This 20 is just arbitrary for now but I am definitely open to suggestions. 🙇🏻♀️
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 think only rendering the top N in a data table sounds interesting! I wouldn't mind trying to implement that.
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.
@myasonik do you have suggestions for larger datasets? I see the top N being kind fo cool and just having somewhere (maybe the table caption?) say the amount of results there were?
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.
@myasonik in line with Rachel's question, it would be interesting to know if there's a good practice for handling the current tradeoffs between accessibility annotations (DOM with ARIA labels etc.) and other goals, as I'm not currently aware of an API that would detect if a screen reader is used. The other goals include, bounded rendering time, responding page, economy of power and CPU utilization, economy with browser memory. Unfortunately the resource usage of DOM rendering is roughly 100x larger than that of DOM rendering.
Rachel I'm also thinking that maybe, reaching a cap, a "slice row" for "Other" could be created, with a single, longer text description for screen reading. While that can still be a long string, it'd establish a bounded DOM element count. DOM and general webpage performance worsens with every DOM element, even if those DOM elements do nothing.
Rachel, another important thing to do is to ensure that the table and row creation do not cause undue layout work for the browser. Depending on a few factors, table rendering speed can vary wildly. The rule of thumb is that if the columns are of fixed width, then the layouting is quite fast, but if the column widths are driven by the table contents, which I believe is the default, then it becomes a much heavier task. Let's chat about this when we look at the PR together.
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.
Was just going back through this PR and thought I'd document the outcome of this (we 3 chatted on zoom): In short, we're only rendering some subset of data at first (until a user interacts with a "load all" button) so there shouldn't be consequential impact to performance.