Open batpad opened 10 months ago
The option["display_name"]
is no longer used when choices
is unset. Not sure what to do about this, but it should be documented and the test cases / documented examples using unlisted_choice
without choices
shouldn't configure it.
'profile_options': {
'image': {
'display_name': 'Image',
'unlisted_choice': {
'enabled': True,
'display_name': 'Image Location',
'kubespawner_override': {'image': '{value}'},
}
},
},
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?
Consider 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.
The option["display_name"] is no longer used when choices is unset. Not sure what to do about this, but it should be documented and the test cases / documented examples using unlisted_choice without choices shouldn't configure it.
Good catch.
Consider if admin admin wants to default to jupyter/base-notebook:latest, unless the user specifies something else via unlisted_choice specifically.
Hmmm. What if we:
choices
or a user selects "Other" in the drop-down and leaves the input blank.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?
I suggest we add unlisted_choice.default_value
(string / Unicode) config and how it work like this.
unlisted_choice.default_value is None
required
to the input tagunlisted_choice.default_value is not None
placeholder=<the default value>
to the input tagunlisted_choice.default_value
should only be used if there are no other pre-defined choices, or if there are other pre-defined choices but unlisted_choice was explicitly chosen in the dropdown listLet's consider these three situations with only unlisted_choice
used.
unlisted_choice.default_value
is Nonerequired
attribute is addedunlisted_choice.default_value
is not Noneplaceholder=<default value>
attribute is addedAre 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 unlisted_choice.default_value
interactions with choices
configured:
@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 unlisted_choice.default_value
configuration option, with the behaviour described in your note above @consideRatio .
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?
@yuvipanda - this PR should now correctly hide the Select drop-down if there is only an unlisted_choice
and no choices
specified. It probably needs a bit of testing, but this should "work" to visually hide that drop-down if there is only an unlisted_choice
.
What this does not add yet is the default_value
handling per comments above. I feel like I'm not able to fully make sense of the trade-offs of not including that or including that as a separate follow-up PR. That does seem like a bit more involved work including backend with tests - I could dig into it but would really much rather prioritize the much-needed larger refactor of this frontend code. Happy to chat!
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