-
Notifications
You must be signed in to change notification settings - Fork 304
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
Refs #756: Handle no choices provided if unlisted_choice is present #778
base: main
Are you sure you want to change the base?
Conversation
f262981
to
9efac4f
Compare
The
The key question in my mind is about handling what it means to submit an unlisted_choice without providing a value. I'm not sure what I think should be allowed and should happen. Hmm.... In an example like above where image is configured, it really doesn't make sense for a submission to be allowed - but that is the responsibility of the admin configuring this I think. The question becomes if kubespawner_override ever allow a blank submission of an unlisted_choice, and when that happens, what should be expected to happen? ScenarioConsider if admin admin wants to default to jupyter/base-notebook:latest, unless the user specifies something else via unlisted_choice specifically. I can see that be relevant. How would we go about supporting that? I think the options are:
I'm not sure what I think works out best. I think currently, we the behavior corresponds to the second option. |
Good catch.
Hmmm. What if we:
Admittedly, I've not fully thought this through, but that seems like it might be expected user behaviour, and if we expect the field to have a value, we should just validate that on the frontend? |
Implementation proposalI suggest we add
MotivationLet's consider these three situations with only
Are there use cases for all of these situations strong enought to motivate the added complexity of supporting them? I think yes for situation 1 and 2, but for situation 3? I don't think so currently. If a jupyterhub admin provides a way to configure something via an unlisted_choice, they know what that something they want to configure is. Knowing what that something is, it will be fine in almost all situations to make them to fall back to configuring an explicit default value instead of doing nothing. The edge case I can imagine is that you may want to not explicitly configure something as a default. I'm more concerned about providing an implementation to support situation 3 now than not. If we do it, we add complexity for a hypotetical use case, not just for maintainers, but for users that needs to understand our docs. Due to this, I propose we disregard situation 3 for now. Let's finally consider the
|
@consideRatio this all sounds good. Am afraid that I may not have the capacity to take this on this month :( - I thought I could get it done, but am at a team week this week and then vacation for 2 weeks. Just don't want to hold up a release, etc. - if someone else is able to take this on, that's amazing - else I can definitely come back to this toward the end of the month. Big apologies! |
My suggestion @consideRatio is to not block 6.1.0 release on this, and let @batpad come back to this after his vacation. I'm hopeful to grow contributor base :) |
Trying to pick this up: I forget a bit larger context here. But from the comment above, it seems like the idea is to add an I'm a bit worried about what this might involve in terms of backend validation, but I can find some time to work on this this week. @yuvipanda @consideRatio do you know / remember if there's anything else to do here to close this out? Thank you! |
Got any more time to work on this, @batpad? |
9efac4f
to
b829054
Compare
for more information, see https://pre-commit.ci
@yuvipanda - this PR should now correctly hide the Select drop-down if there is only an What this does not add yet is the |
Allows
choices
to be empty ifunlisted_choice
is present - handles hiding the select dropdown ifchoices
is empty.Because of the toggling we do for the hidden / inactive state of the
unlisted_choice
input, some of the frontend code is a bit awkward. I can think about this, but I think a more elegant solution might require a more complete refactor / moving to a more JS-based approach of handling options and rendering.I think this works, and I tried to add a basic test for this case, but I do worry a bit about edge cases - this probably needs a bit of thorough testing.
cc @consideRatio @yuvipanda @GeorgianaElena