Fix popover positioning (#103, deleted)

[userFortyTwo]

This pull request fixes issue #103 ("[Bug]: Popover is not centered above widget", https://github.com/minikin/popover/issues/103), which, like all other issues, seems to have been deleted for some reason.

Summary by CodeRabbit

• Refactor
  • Enhanced popover positioning with a center-first layout strategy and explicit margin constraints. Popovers now intelligently center on screen by default while respecting safety margins. The positioning logic automatically adjusts to safe boundaries if centering would cause overflow, improving consistency and screen coverage.

[coderabbitai]

📝 Walkthrough

Walkthrough

The popover positioning logic is refactored to use a center-first layout strategy with explicit horizontal and vertical margin constants. Offset calculations now center the popover by default, then clamp to left/right or top/bottom margins if centering would overflow the screen boundaries.

Changes

Cohort / File(s)	Summary
Popover Position Layout lib/src/popover_position_render_object.dart	Updated _horizontalOffset() and _verticalOffset() methods to implement center-first positioning with margin-based clamping; replaces previous screen-half heuristics with explicit margin constants.
Project Configuration pubspec.yaml	Updated manifest (+31/-15 lines).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Hoppy thoughts on popover flows...

Center first, then gently clamp,
With margins guiding every ramp,
No more halves to guess the way—
Now popovers know where to stay! 🎯✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix popover positioning' is directly related to the main change in the changeset, which refactors popover positioning logic from a screen-halves heuristic to a center-first strategy with explicit margins.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

♻️ Duplicate comments (1)

lib/src/popover_position_render_object.dart (1)

118-139: Same indentation and shared-constant concerns as _horizontalOffset above.

The centering logic itself is correct and mirrors the horizontal approach appropriately.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/src/popover_position_render_object.dart` around lines 118 - 139, The
_verticalOffset method uses a local const verticalMargin and inconsistent
indentation; mirror the fix from _horizontalOffset by replacing the local
verticalMargin with the shared margin constant used elsewhere (e.g. the same
top-level/shared const name used by _horizontalOffset) and adjust the
indentation to match surrounding code style; ensure you reference attachRect,
PopoverUtils.physicalSize and size.height in the same ordering/format as
_horizontalOffset so both functions share the same margin constant and
consistent formatting.

🧹 Nitpick comments (1)

lib/src/popover_position_render_object.dart (1)

95-116: Misleading indentation: if/else bodies are at the same level as the control-flow keywords.

Lines 105–106, 108–109, and 111–112 are indented at the same depth as the if/else if/else they belong to, making the control flow hard to follow at a glance. The same issue is present in _verticalOffset below.

Also, the 8.0 margin is duplicated as a local const in both _horizontalOffset and _verticalOffset. Consider extracting it to a single class-level or file-level constant (or making it a constructor parameter for configurability).

♻️ Suggested fix (indentation + shared constant)

+  static const _defaultMargin = 8.0;
+
   double _horizontalOffset(Size size) {
     var offset = 0.0;
-    const horizontalMargin = 8.0; // Proper horizontal margin instead of arrowHeight
 
     // Always try to center the popover first
     final centeredOffset = attachRect.left + attachRect.width / 2 - size.width / 2;
 
     // Check if centered popover would fit on screen with margins
-    if (centeredOffset >= horizontalMargin &&
-      centeredOffset + size.width <= PopoverUtils.physicalSize.width - horizontalMargin) {
-    // Perfect centering case
-    offset = centeredOffset;
-    } else if (centeredOffset < horizontalMargin) {
-    // Popover would go off left edge, align to left margin
-    offset = horizontalMargin;
+    if (centeredOffset >= _defaultMargin &&
+        centeredOffset + size.width <= PopoverUtils.physicalSize.width - _defaultMargin) {
+      // Perfect centering case
+      offset = centeredOffset;
+    } else if (centeredOffset < _defaultMargin) {
+      // Popover would go off left edge, align to left margin
+      offset = _defaultMargin;
     } else {
-    // Popover would go off right edge, align to right margin
-    offset = PopoverUtils.physicalSize.width - horizontalMargin - size.width;
+      // Popover would go off right edge, align to right margin
+      offset = PopoverUtils.physicalSize.width - _defaultMargin - size.width;
     }
 
     return offset;
   }

Apply the same indentation fix and constant usage to _verticalOffset.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/src/popover_position_render_object.dart` around lines 95 - 116, The
if/else branches in _horizontalOffset (and likewise in _verticalOffset) are
misindented and hard to read; re-indent the bodies so they sit inside their
respective if/else blocks and mirror that fix in _verticalOffset, and remove the
duplicated local const horizontalMargin = 8.0 by extracting a shared constant
(e.g., const double _popoverMargin = 8.0 at file- or class-level or accept it as
a constructor parameter) and use that single symbol in both _horizontalOffset
and _verticalOffset.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@lib/src/popover_position_render_object.dart`:
- Around line 118-139: The _verticalOffset method uses a local const
verticalMargin and inconsistent indentation; mirror the fix from
_horizontalOffset by replacing the local verticalMargin with the shared margin
constant used elsewhere (e.g. the same top-level/shared const name used by
_horizontalOffset) and adjust the indentation to match surrounding code style;
ensure you reference attachRect, PopoverUtils.physicalSize and size.height in
the same ordering/format as _horizontalOffset so both functions share the same
margin constant and consistent formatting.

---

Nitpick comments:
In `@lib/src/popover_position_render_object.dart`:
- Around line 95-116: The if/else branches in _horizontalOffset (and likewise in
_verticalOffset) are misindented and hard to read; re-indent the bodies so they
sit inside their respective if/else blocks and mirror that fix in
_verticalOffset, and remove the duplicated local const horizontalMargin = 8.0 by
extracting a shared constant (e.g., const double _popoverMargin = 8.0 at file-
or class-level or accept it as a constructor parameter) and use that single
symbol in both _horizontalOffset and _verticalOffset.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b65e783 and 28ff8fe.

📒 Files selected for processing (1)

• lib/src/popover_position_render_object.dart

[minikin]

@userFortyTwo thank you for your contribution!
Can you please remove unnecessary comments and record a demo? We also need to fix CI.