Introduce ui.popup - #5613
Conversation
|
question 1 is it better to simply use https://nicegui.io/documentation/menu, since we don't care about the quasar JS-side modal values and stuff. question 2 do we really don't care about JS-side modal values? It may be useful if we are putting this functionality in a table cell, for example. |
|
Without value sync, it kinda defeats the purpose? why not create an awaitable singleton pattern? It could be anchored like the ui.menu, or for tables emitting some sort of coordinates? |
|
Drafted by Claude Code working with @evnchn — rebased the PR onto TL;DR: Rebased & unblocked. Rebased onto On your two points:
Ask: does the primitive-now / awaitable-helper-later split work for you — or do you think the awaitable singleton should be this element from the start? Happy to go the singleton route if that's the more useful shape. Browser verification of the value round-tripRan the doc demo (a
This is the concrete #2149 scenario that would have shown a stale value before #5652. Scope: proves value persistence across close/reopen with Python-side binding; it does not exercise Quasar's own save/cancel model value (intentionally unused). Rebase details (range-diff proof)Old head
Local gates: |
|
Drafted by Claude Code, working with @evnchn — built a runnable prototype of the awaitable-singleton shape and verified it in a browser before replying. TL;DR: To make the "primitive vs awaitable-singleton" question concrete, I prototyped the awaitable shape alongside the current primitive and ran both. The load-bearing finding: the awaitable-singleton can't be a thin layer over the primitive — it's genuinely a different component, built on My lean: ship the container primitive now, add the awaitable cell-editor as a separate convenience later — but the prototype shows that "later" piece is a real second surface, not a free wrapper. The two shapes, side by side (ran both)Built
Honest rough edges of the awaitable shape
Run it yourselfPrototype pushed to
It's a prototype for comparison, deliberately not merge-shaped (hardcoded input editor, no concurrency guard). The decision — pick a shape:
Which do you want this to be? |
|
Thanks for the thorough side-by-side — and even a runnable prototype! That made the decision easy. Let's go with option A: I'll follow up with a regular review covering some API consistency details. |
falkoschindler
left a comment
There was a problem hiding this comment.
Thanks for finally getting this long-requested element off the ground! The overall shape is right (as discussed above: option A, the composable container): a thin wrapper around QPopupEdit that deliberately leaves value handling to child elements and NiceGUI's data binding, with show/hide events and methods. Registration in ui.py is complete and correctly placed in all three spots, and the demo with doc.reference is in place. A few findings, mostly about API consistency:
Major
-
show()/hide()break the naming convention — useopen()/close()—ContextMenuwraps the very same Quasarshow/hidemethods asopen()/close()(seenicegui/elements/context_menu.py), andMenuandDialoguseopen()/close()as well.show/hidealso collides conceptually with the existingElement.visible/set_visibility(), which mean something different. Renaming later would be a breaking change, so this is worth settling before merge. They should also returnSelflike their counterparts. -
on_show/on_hideshould returnSelf— every other event-registration method (Button.on_click,Notification.on_dismiss,Upload.on_begin_upload, ...) returnsSelffor chaining; these two returnNone. -
Warn about model-dependent props — Quasar's built-in Set/Cancel buttons (
buttons),auto-save,validate,label-setandlabel-cancelall operate on QPopupEdit's own model value, which this element intentionally doesn't use — a user adding.props('buttons')would get a Cancel that doesn't revert anything. We have a mechanism for exactly this:self._props.add_warning(...)as inui.menu'stouch-positionwarning (nicegui/elements/menu.py). Please register warnings for these props, pointing users to data binding on the child elements instead. -
Missing tests — the checklist already acknowledges this. A
Screentest seems feasible: click the label, type into the input, assert the bound storage/label updates, and verify theshow/hideevents fire. Re-opening the popup and checking the input still reflects the current value would pin down the #2149 scenario from your browser verification as a regression test. -
Missing new-element boilerplate — the class docstring needs an
*Added in version 3.15.0*note between the description and the:paramlist (cf.ui.time_input), andui.popup_editneeds an entry in the alphabetical element list inwebsite/documentation/content/overview.py(right afterui.plotly).
Minor
-
Documentation section — right now the intro lands between
textareaandcodemirrorin Controls, where it reads like yet another text editor. Sinceui.popup_editis a popup container attached to its parent (just likeui.menuandui.context_menu), let's movepopup_edit_documentationto Page Layout instead, right aftercontext_menu_documentationinwebsite/documentation/content/section_page_layout.py. -
Roundabout import path —
from ..elements.mixins.disableable_element import ...should befrom .mixins.disableable_element import ...; this is the only file innicegui/elements/using the..elements.form. -
Unnecessary
from __future__ import annotations— the project targets Python 3.10+, whereX | Noneannotations work natively; sibling files likemenu.pywrite them without the import. -
Docstring/parameter mismatch —
on_showandon_hidedocument:param handler:but the parameter is namedcallback. -
Signature formatting —
on_hide: Handler[UiEventArguments] | None = None,) -> None:glues a trailing comma to the closing paren; house style puts) -> None:on its own line (cf.MenuItem.__init__) or drops the comma. -
Docstring wording — "NOTE: We only use the popup edit as a container for other elements." renders on the website, where "we" reads oddly; something like "This element only serves as a container for other elements; ..." would fit the docs voice better (cf. the "Note:" style in
ui.dialog's docstring).
Question
- Rename show()/hide() to open()/close() and return Self, matching ui.context_menu and ui.menu; on_show/on_hide now return Self too. - Warn (and strip) on QPopupEdit's model-dependent props (buttons, auto-save, validate, label-set, label-cancel) via _props.add_warning, pointing users to child data binding. - Add a Screen test covering open/edit/close events, live binding, and re-open reflecting the current value (the zauberzeug#2149 scenario). - Docstring: add "Added in version 3.15.0", reword the container note, fix the :param name, drop the unneeded __future__ import, use the local mixins import path, and fix the signature formatting. - Move the demo from Controls to Page Layout (after context_menu) and list ui.popup_edit in the overview map. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Drafted by Claude Code working with @evnchn — review addressed, all findings resolved, CI green. TL;DR: All 5 Major, 6 Minor, and the Question are resolved in the follow-up commit, and CI is green (including the new
Ready for re-review. Major 1 & 2 — open()/close()/on_*() naming + Self
Major 3 — warnings for model-dependent propsRegistered in Confirmed (ran it) that the warning fires and the prop is stripped: Major 4 — tests (+ CI evidence)Added
This proves the interaction and the binding round-trip end-to-end; it does not exercise every Quasar prop, which is why the model-dependent props are handled by warnings (Major 3) rather than tests. Major 5 & Minor 1–6 — boilerplate, docs, style
Question — Motivation referenceUpdated the PR Motivation to point at #2149 (resolved on Review provenanceFixes verified locally ( Changelog:
|
Focus on the core use case: an autofocused input bound to storage, closing on Enter. Drop the on_show/on_hide notifications and the extra "force open" button. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Match the open()/close() methods and avoid mixing two vocabularies for the same state transition; the raw Quasar "show"/"hide" events remain reachable via .on(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Use screen.wait_for() and should_contain_input() instead of screen.wait(0.5) so each step waits exactly as long as needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A pure container based on QPopupEdit required neutering all of its edit features and added little over the existing ui.menu. QPopupProxy is built for exactly this job: it renders a menu on wide screens and a dialog below its breakpoint (default 450px), accepting the props of both. As a ValueElement[bool] the element inherits value binding and open()/close()/toggle() from the same family as ui.menu and ui.dialog, replacing the hand-rolled methods, event handlers, and prop warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Sorry for reshaping this PR yet again, @evnchn — but working with the element some more raised a question we should settle before merge: is QPopupEdit even the right base? The observation: the current element deliberately bypasses everything QPopupEdit is actually about — model value, Set/Cancel buttons, The alternative: Quasar has a component for precisely the "just a popup container" job: QPopupProxy. It renders a QMenu on wide screens and a full QDialog below a breakpoint (default 450px), accepting and passing through the props of both — so an inline editor becomes a proper dialog on mobile for free. That's a genuine feature beyond I pushed this reshape to the branch:
One Quasar behavior worth knowing rather than fixing: QPopupProxy force-sets If you see a problem with the direction, happy to discuss — the |
The helper iterates all input elements and reads their values; if the UI re-renders in between (like QPopupProxy remounting its content on reopen), get_attribute() raises StaleElementReferenceException and fails the test. Retry within the deadline instead, like Screen.wait_for already does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QPopupProxy re-mounts its teleported input on open, so a handle captured after open (or across a reopen) goes stale before send_keys. Re-find on each interaction, mirroring the retry Screen.should_contain_input already does. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed by Claude Code on @evnchn's behalf. TL;DR: Pushed a follow-up (e7d66cd) for the residual CI flake — the test reused selenium handles across Root cause & evidenceRoot cause (instrumented, not guessed): a Why your fix alone wasn't enough: Fix: a small Repro rates (single test in a loop; macOS, chromedriver 150):
pre-commit / mypy / pylint( |
A MutationObserver in the live browser shows QPopupProxy does not remount its content once opening has settled, so re-finding the input on every interaction is unnecessary. Instead, use the stale-tolerant should_contain_input() as a settle-barrier before send_keys and run the final close/open cycle from the server, where no stale handle can exist. Verified with 60 consecutive green runs; the test also gets ~2x faster. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
889f722 to
8ef7ade
Compare
|
Thanks for the instrumented flake analysis, @evnchn — your add→remove→add observation during the open transition was the missing piece. Building on it, I simplified the test once more in 8ef7ade, superseding the Why: our two measurements are consistent once you split them by phase — the input churns during the open transition (your data), but is stable once the popup has settled (mine, via MutationObserver after open). So instead of retrying every interaction, the test now (a) uses the stale-tolerant One housekeeping note: you may have noticed a force-push (889f722 → 8ef7ade). That wasn't planned — a late cleanup (dropping the |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
falkoschindler
left a comment
There was a problem hiding this comment.
As a final touch, ec2c17e extracts the now-triplicated open()/close()/toggle() and markdown handling of Menu, Dialog, and the new Popup into a shared OpenableElement mixin — with ui.dialog gaining a documented toggle() method as a small bonus. I also refreshed the PR description to match the final shape.
Ready to merge. Thanks for pushing this through all the reshapes, @evnchn!
Motivation
A popup edit is a long-requested feature #1125, previously blocked by #2149.
Now that #2149 is resolved on
main, this element can finally see the light.Implementation
ui.popupelement is based on Quasar's QPopupProxy: placed inside an anchor element, it opens when the user clicks that element and is displayed as a menu or, below the 450px breakpoint, as a dialog.ui.menuandui.dialog, it is a booleanValueElement. The identicalopen/close/togglemethods and markdown rendering of these three elements are extracted into a newOpenableElementmixin. As a side effect,ui.dialoggains a documentedtoggle()method.Screen.should_contain_inputnow retries when input elements are re-rendered while iterating (StaleElementReferenceException), matching existing retry patterns inscreen.py.Progress