-
-
Notifications
You must be signed in to change notification settings - Fork 902
Improve ChoiceElement's options by making them richer and typed better
#5322
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
|
Imagine me when I opened GitHub and see this
Totally thought instinctively that this was some prank PR and wanna close it at first. Looking closelier I see it is a recarnation of #4777. That's good! Tell us when it's closer to done. Add oil! 👍 |
|
haha Yeah, feel free to ignore until it's out of draft, @evnchn. I'm still planning on starting a discussion soon and use these changes as examples of the benefits. |
|
Question: For the typing enhancement on |
Yes @evnchn, that's the goal. I'm still learning how all the components work with the I also got the docs site working locally and added an example based on these changes. Screen.Recording.2025-10-23.at.11.29.27.AM.mov |
ChoiceElement options richer and improve typing
ChoiceElement options richer and improve typingChoiceElement's options richer and improve typing
|
And also for the changes in If that is the case then maybe we should prioritize delivering that since the impact is higher. |
dcddab6 to
d093738
Compare
| def test_id_generator(screen: Screen): | ||
| @ui.page('/') | ||
| def page(): | ||
| options = {'a': 'A', 'b': 'B', 'c': 'C'} | ||
| select = ui.select(options, value='b', new_value_mode='add', key_generator=lambda _: len(options)) | ||
| ui.label().bind_text_from(select, 'options', lambda v: f'options = {v}') | ||
|
|
||
| screen.open('/') | ||
| screen.find_by_tag('input').send_keys(Keys.BACKSPACE + 'd') | ||
| screen.wait(0.5) | ||
| screen.find_by_tag('input').send_keys(Keys.ENTER) | ||
| screen.should_contain("options = {'a': 'A', 'b': 'B', 'c': 'C', 3: 'd'}") |
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 it makes sense that I deleted this test but I probably should add tests for new_value_to_option.
…lowing for json primitive types to still be passed.
41357fd to
246e155
Compare
…hoice components.
ChoiceElement's options richer and improve typingChoiceElement's options by making them richer and typed better
@evnchn, I'd consider working on a separate PR just for that if that's what you guys want. I've also taken this PR out of draft so people can start looking at it; however, I still need to make a few more tweaks/changes for it to be "ready" (see the |
…ot tests/test_user_simulation.py udpated and passing based on the changes.
I mean cherry-pick changes later, which should be moderately easy to do. Also it's more of a last resort - that really happens when the rest of this PR's code doesn't get an OK...
May not be good, a bit noisy at times... |
@evnchn, I put it back in the "draft" state based on your comment. I'm currently working on the PR description since all tests are now passing and I updated/added doc examples. Would you mind letting me know if what I have on the PR description right now clear to you? I'd really like to make it as clear as possible. |
|
@jdoiro3 I see you made the call signature, at one point, into That doesn't seem very Pythonic to me, may be a bit clumsy to write. If possible, let's avoid it? On trying to resolve that, I'm checking where else in our code we have "better typing" and I find the Event system quite well-written on that regard. https://nicegui.io/documentation/event
Line 22 in b72e7bb
Would that help in this PR? Maybe we can have |
…code changes. I'm still not happy with some of the code but this is closer.
|
I'm not sure this PR is going to work out. I can't figure out how to get the typing working properly for I do think allowing users to pass an |
|
If, worst case scenario, we cannot make it work without a breaking change, then it has to wait for 4.0. But I don't think people will like that release that way... I'm a bit occupied right now but I think we can take another shot later. |
|
@jdoiro3 Thank you for the effort put into this PR and the exploration of improving type safety for choice elements. Considering the discussion and the code changes, I'd like to suggest closing this PR for the following reasons:
If there's interest in typed options in the future, a backward-compatible approach that doesn't change existing behavior would be preferable. Thanks again for the contribution! |



Note
PR description is still a work in progress.
Motivation
This PR improves choice elements (e.g.,
ui.select,ui.toggle,ui.radio, etc.) by making the options a type and not an implementation detail. It also improves the typing support for these components, allowing static type checkers to help users avoid bugs, make code more readable and making it easier to utilize custom quasar slotting.Changes to the
ui.selectelementTip
click to expand sections containing full code examples before and after these changes.
Before
on_value_change's eventvalueisAnyso type checkers can't help us when we try to add anintto astr.Any | Noneso static type checkers can't help us.ints, when adding new values the event is astrwhich can cause issues.After
ui.selectelement is now type aware. The first type is the value type and the second is the option type.Personoption, containing an icon and caption in addition to the required label field, astrand the value field, anint. And since the options structure is no longer an implementation detail, it's a lot easier to use the various option fields when slotting.ui.selectelement correctly know that the option is of typePerson.Implementation
Progress
TODO
dict[L, V]to be passed as options. This will also allow the PR's changes to test code to be minimal.