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
Added XDMoD analytics metrics to jobs widget #3789
Added XDMoD analytics metrics to jobs widget #3789
Changes from 7 commits
b669c9c
c91cc10
6bb921e
cab4581
99f3656
bd22a41
30ff532
773f9f1
7bfd405
2a649ca
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.
This should have a heading. You can use the class
sr-only
, but I think actual text here could be nice too (if it's a bit short/succinct).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.
will add some text
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 there's something odd about creating a new table row here. Screen readers will get tripped up when row N (a row they've looked at) is now suddenly different because it was the initial row, but after this expansion it's now this new data.
Shouldn't it put all this stuff in the original
td
and just use some styling to achieve the same look & feel?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 can improve the accessibility of the hidden rows with
aria-hidden
andaria-expanded
andaria-controls
. Wouldn't that address the screen readers issue?I am not sure how feasible is to add content to the original td that contains the +/- icon and make it break out of the existing table structure, push the other rows down, and appear in the same way. Or even to have a hidden td and do the same. Will need to investigate.
It would be more feasible to appear as a small pop up / tooltip, but I think that would be less user friendly.
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.
No it's the fact that it's a
tr
that's being created. Imagine this - you saw row 2 which the screen reader read out asrow 2
. Then you expand this thing in row 1 and now all the sudden row 2 is this new information, not what you'd previously read through.Yea I'm not sure it'd be easy with the way it is now, but I'm sure there's a
flex-flow
orflex-direction
trick we can employ here. Or maybe it'd be easier if expanding thetd
shifts everything to the right down so the new information shows on top of the row instead of on the bottom?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 have removed the extra
tr
for the analytics data. I am using now atd
to render and display the contents.