-
Notifications
You must be signed in to change notification settings - Fork 30
GScan sidebar: task states badges #2331
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
Conversation
|
LGTM. A nice hybrid of the job icons (which are better for monitoring) and the task icons (which are truer to the meaning of the data). Some small comments:
|
There is a disparity between the offline/mock mode and real life as succeeded tasks do not show in the sidebar in real life
I tend to disagree - these are badges not necessarily circles (though aesthetically they should be circles when at minimum width). E.g. see https://vuetifyjs.com/en/components/badges (though we couldn't use this as the colour blind theme relies on outlines which are not supported by v-badge) |
Two reasons they should be circles:
|
|
I have copied the deltas from the current parallel suite and this is what it would look like. IMO easier to read as I have got rid of empty states Edit: moved screenshots to OP |
|
Aah, so you've also removed the empty states. Not so sure about that. The grid has monitoring and accessibility advantages. |
94ffc67 to
6de7a9f
Compare
|
Agreed this is a nice compromise between the status quo and what I tried to do on #1050. I don't like the fact that we chose to use job state icons for task states in the sidebar. This colours look good, but it definitely confuses users sometimes: "there's a bug in the GUI: my tree view has red failed tasks [jobs!] but the side bar is not highlighting them!". The monochromatic task state icons would not work for this kind of visual simplification though, and the badges on this branch are at least not identical to the job icons which makes it more legit to say they represent task states not job states. I really like what @MetRonnie has done in the screen shot above - it is clean and simple, and only shows the important info. I don't think we should retain the grid - it is busy and cluttered by comparison. And ditching the grid allows badges that expand with the number of digits - which is more appealing than fat badges that likely have only single digits most of the time. Parsing (by eye) the grid on the left is definitely more difficult. I don't see any great advantage to aligning all the states like that, when most of them are inactive most of the time. Summary: IMO this PR is a clear win as-is. |
|
I was going to comment last week; having the number/count at a glance is a welcome improvement.. One thing, which is hinted by Hilary's 1050 proposal, and then skipped over with
Is that; aligning had the advantage of being workable by those who aren't good/capable with colours (because you knew/learnt which column was associated with what state). So, I don't know if it's something we need to consider, if it's not too difficult to make it optional then maybe we should? Perhaps not square, if like the old/left, but at minimum invisible margins using the new and/or a faded/translucent circles with zero or no number (when no tasks of associated state)?
Maybe this would be an easy win, so the ordered empty placeholders are always present (optionally). |
|
IMO a rather unwieldy grid is not well justified by accessibility concerns for a small minority of users. Having to remember the column number of each state is pretty dire! Better to make something clear and clean, and figure out how to tweak that for color-blindness - so how about exactly what @MetRonnie has done here, but (for color-blind theming) replace the job-state-color badges with task state icons with the number beside (or above, or partially overlapping, or whatever) instead of inside? |
|
Not sure I get the issue with the grid, and colourblindness aside, I find the grid layout helpful. But happy to be outvoted. I don't think any docs changes are needed, however, it would be good to add a changes article for this. |
|
Thanks @oliver-sanders :-) |
Well, there's only 4 columns, so it'd be fairly automatic if you were used to looking at it this way.. But point taken. It's not a big gripe, and doesn't effect our operational team |
|
Consider this approved from a conceptual and functional perspective, but I haven't had a chance to look at the code yet... |
ChrisPaulBennett
left a comment
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.
LGTM, no notes.
|
Note these task state badges follow the colour theme set in the user profile settings, just like job icons. So colour blindness has been taken care of. |
662dfa3 to
173af9c
Compare
|
Given @hjoliver's approval in principle, I'll take over as technical reviewer as we could really do with getting this into use. |
oliver-sanders
left a comment
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.
LGTM.
We didn't get agreement to remove the "latest tasks" content in the tooltip and this PR doesn't add any alternative functionality to fill the gap. So I think we need to back out of this change (or come up with a better solution).
173af9c to
5cfffc3
Compare
5cfffc3 to
266038f
Compare
1919acd to
77611ad
Compare
We cannot show the true latest tasks for groups, and it is unclear what workflow each task belongs to
77611ad to
6cd68ef
Compare
Supersedes #1050
Partially addresses #2313
Old (left), new (right):
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.