-
Notifications
You must be signed in to change notification settings - Fork 4
runtime version scanner: improve table display and filter controls #173
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
|
Some comments from using this (I haven't read the code doing this, so some of this is knowable if I RTFS, but I wanted to approach this round of feedback as a user): This is getting pretty close to something useful + complete.
|
I miss the sliders a little bit, but the dropdowns are simpler to use. One weirdness that comes out of the For example: I have R content with I understand you said you wanted to make that an input rather than Having the total for pagination would be great if you have that available. I also don't know what "Highlight stale builds" means. I assume this is the content was built on an older version of the language and Connect would use something different now? Definitely need some explanation on that. |
dotNomad
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.
The code here looks great. I highlighted a couple bits for feedback, but totally fine with any of those being follow-up PRs 😄
| color = "#7D1A03", # Red color for versions not on server | ||
| fontWeight = "bold", | ||
| background = "#FFF0F0" # Light red background | ||
| )) | ||
| } else { | ||
| return(list( | ||
| color = "#276749" # Green color for versions on server |
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.
Nice job on the colors here 👍 They are accessible which is tough for color-on-color 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 actually think that highlighting in color is perhaps a feature which will go away (it doesn't really communicate something easily understandable) but thanks 😂
| conditionalPanel( | ||
| condition = "input.use_r_cutoff == true", | ||
| sliderTextInput( | ||
| "min_r_version", | ||
| selectizeInput( | ||
| "r_version_cutoff", | ||
| label = NULL, | ||
| choices = "...", | ||
| pre = "≤ " | ||
| choices = NULL | ||
| ) | ||
| ), |
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 noticed the select boxes here didn't have labels. They are not shown which makes sense, but they aren't accessible due to this change since there is no content in the <label>. If we could supply a label and hide it with some CSS like the below that would be great.
.sr-only {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip: rect(0, 0, 0, 0);
white-space: nowrap;
border-width: 0;
}
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.
Aah, interesting. Good point!
I think that these controls may change a lot more or be replaced entirely, so I'm going to leave myself a reminder to check accessibility before the app is finalized, as new controls may need to be made accessible in a different way, or may not have this issue. Is that ok?
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.
Totally fine with me sounds like a great way to handle that 🫡
|
@jonkeane Thanks for your feedback here! I'll go point by point.
I agree! I had been thinking about some kind of timeline view — it might be fiddly to code but worth it.
Yep, I like these two pieces of feedback.
Possibly! It might be a bit fiddly I'm not sure what to do if there are multiple runtimes selected for the filter, but perhaps we only do it when a single one is selected.
It means "Highlight items whose most recent environment build used a version of a runtime that doesn't exist on the server any more." Communicating better about what this means feels related to better communicating about server versions and the currently selected versions. In the next iteration I'm focusing on different ways to communicate the versions — perhaps a timeline or a histogram. |
Can you punt on this right now? 🚮 the message + code etc. And come back to it when the other stuff is in a better place? That will help you stay focused on getting something useful in this iteration. Like we talked about in our 1:1: this is an edge case, it would be nice to message about it, but this is not the most important thing / first thing we should be polishing. |
Yeah, definitely. I agree, I think that this is not helpful as a core functionality, the current messaging is more confusing than it is helpful, and trying to focus on it or whip it into shape this iteration will distract from the core functionality. |
This PR contains a number of improvements to the runtime version scanner:
With the version selector control — I had wanted to do a "type in the version number" control, but I didn't have time to implement that, as it would have required significant refactoring of how the app handles versions. It might be worth postponing to a future iteration.