Skip to content

refactor: #17674#19453

Merged
mehmetcetin01140 merged 1 commit intomasterfrom
issue-#17674
Mar 2, 2026
Merged

refactor: #17674#19453
mehmetcetin01140 merged 1 commit intomasterfrom
issue-#17674

Conversation

@mehmetcetin01140
Copy link
Copy Markdown
Contributor

fixes #17674

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 2, 2026

Thank you for your contribution! We will review your PR shortly. Please make sure to manually link it to an issue for proper tracking.

@mehmetcetin01140 mehmetcetin01140 merged commit d196a95 into master Mar 2, 2026
3 checks passed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 2, 2026

AI Code Review

Changes Suggested

Summary

PR refactors overlay service to hide overlays when parent dialog is dragged, changing from realigning to hiding approach. Renames realign methods to parentDrag throughout the codebase.

Issue Alignment

The PR attempts to address the issue where overlays (like dropdowns) don't move with their parent dialog when dragged. However, the solution (hiding the overlay) is questionable - users expect the overlay to move with the dialog, not disappear when they try to move the dialog.

Concerns

  • CRITICAL: The solution hides the overlay instead of moving it with the parent - this creates poor UX as the overlay suddenly disappears when user drags the dialog
  • The original issue asks for the dropdown to 'move along with the popup dialog, keeping its relative position' but this implementation just hides it
  • Breaking change: Changes from realigning (repositioning) the overlay to hiding it completely - this fundamentally changes behavior
  • No tests included to verify the new behavior works correctly
  • The renaming from 'realign' to 'parentDrag' makes sense semantically, but the behavioral change needs reconsideration
  • Calling hide() with true as second parameter - the meaning/purpose of this parameter is unclear without seeing the hide() method signature

Suggestions

  • Reconsider the approach: Instead of hiding, the overlay should be repositioned relative to its target when the parent dialog moves
  • Add configuration option to let users choose behavior: hide vs. reposition vs. close
  • Add unit tests to verify the overlay hides when parent dialog is dragged
  • Document the behavior change in CHANGELOG as it's a breaking change
  • Consider adding a comment explaining why hiding is chosen over realigning
  • If hiding is the final approach, consider emitting an event so users can handle the closure programmatically

This is an automated review. A maintainer will provide final approval.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider mousedown to hide overlays

1 participant