Skip to content

Open popover anchored to item - #123

Open
kf7mxe wants to merge 9 commits into
version-7from
openPopoverAnchoredToItem
Open

Open popover anchored to item#123
kf7mxe wants to merge 9 commits into
version-7from
openPopoverAnchoredToItem

Conversation

@kf7mxe

@kf7mxe kf7mxe commented Dec 12, 2025

Copy link
Copy Markdown
Contributor

No description provided.

@UnknownJoe796 UnknownJoe796 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Review (commit 7e2a8d6)Error: Exceeded USD budget (0.5)

@UnknownJoe796 UnknownJoe796 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Review (commit 7e2a8d6)## PR #123: Open popover anchored to item — Review

Summary: Adds an anchor: RView? = null parameter to openPopover across all platforms, allowing popovers to be positioned relative to a view other than the caller. Clean, backward-compatible API change. A few issues worth addressing.


1. Popover opened inside reactive {} block — potentially stacking popovers

File: example-app/src/commonMain/kotlin/com/lightningkite/mppexampleapp/internal/TestingGroundPage.kt:43-52

reactive {
    if (input() == "test") {
        anchorTarget?.openPopover(
            PopoverPreferredDirection.aboveRight,
            anchorTarget
        ) {
            card.text("Trax Bagley")
        }
    } else closeThisPopover()
}

reactive {} blocks re-execute whenever any dependency changes. If some unrelated reactive dependency triggers re-evaluation while input() is still "test", openPopover will be called again, potentially stacking popovers. This is demo code, but it showcases a pattern users may copy. Consider guarding against duplicate opens (e.g., track whether the popover is already open) or documenting that this pattern needs care.

Confidence: Medium


2. Redundant anchor parameter in demo code

File: example-app/src/commonMain/kotlin/com/lightningkite/mppexampleapp/internal/TestingGroundPage.kt:45-47

anchorTarget?.openPopover(
    PopoverPreferredDirection.aboveRight,
    anchorTarget  // <-- redundant: openPopover is already called on anchorTarget
) { ... }

Since openPopover is called on anchorTarget, the anchor ?: this@openPopover fallback in each platform implementation already resolves to anchorTarget. Passing it explicitly is redundant and makes the API demo confusing — it doesn't demonstrate the use case where the trigger view differs from the anchor view. The PopoverTestingPage.kt:75 example does this correctly (called on a button, anchored to a different button).

Suggested fix: Remove the redundant parameter:

anchorTarget?.openPopover(PopoverPreferredDirection.aboveRight) {
    card.text("Trax Bagley")
}

Confidence: High


3. Inconsistency: atTopStart in MenuButton but not in openPopover (Android)

File: library/src/androidMain/kotlin/com/lightningkite/kiteui/views/direct/MenuButton.android.kt:43 vs library/src/androidMain/kotlin/com/lightningkite/kiteui/views/direct/openPopover.android.kt:51

MenuButton uses atTopStart.onNext(PopoverSemantic).frame { configurePopoverLayout(...) } while openPopover uses PopoverSemantic.onNext.frame { configurePopoverLayout(...) }. Both call configurePopoverLayout which sets Gravity.TOP | Gravity.LEFT, making atTopStart in MenuButton redundant. The positioning should work the same way either way, but the inconsistency between the two callers is confusing.

Suggested fix: Either add atTopStart to openPopover for consistency, or remove it from MenuButton since configurePopoverLayout handles it.

Confidence: Medium


4. Unused import in test page

File: example-app/src/commonMain/kotlin/com/lightningkite/mppexampleapp/internal/TestingGroundPage.kt:5

import com.lightningkite.kiteui.models.ThemeDerivation.Companion.invoke

This import doesn't appear to be used in the file. Likely an artifact from auto-formatting or IDE.

Confidence: High

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants