fix(#3455,#3450): Updated top and bottom positioning for Popover#3477
fix(#3455,#3450): Updated top and bottom positioning for Popover#3477ArakTaiRoth wants to merge 1 commit intodevfrom
Conversation
✅ Deploy Preview for goa-design-2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
willcodeforcoffee
force-pushed
the
dustin/3455
branch
2 times, most recently
from
5514766 to
7728e94
Compare
willcodeforcoffee
force-pushed
the
dustin/3455
branch
from
7728e94 to
52238a8
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes two issues with the Popover component: Dropdown not expanding outside Container (#3450) and AppHeaderMenu not expanding to max width (#3455). The fix changes the positioning strategy from using bottom CSS property with a position: relative wrapper to using top CSS property with absolute positioning relative to the offsetParent. This allows popovers to properly escape container boundaries with overflow properties.
Changes:
- Updated Popover positioning logic to use
topinstead ofbottomand calculate position relative to offsetParent - Removed
position: relativestyle from popover wrapper div to allow proper absolute positioning - Updated test assertions to reflect new positioning approach (checking
style.topinstead ofstyle.bottom) - Added test pages in both React and Angular to demonstrate the fix
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/web-components/src/components/popover/Popover.svelte | Core fix: calculates top position using offsetParent, removes relative positioning wrapper |
| libs/react-components/specs/popover.browser.spec.tsx | Updates test assertions from bottom to top positioning, adds window dimension mocking |
| apps/prs/react/src/routes/bugs/bug3450.tsx | New test page demonstrating Dropdown and DatePicker expanding outside Container |
| apps/prs/react/src/main.tsx | Adds routing for bug3450 test page |
| apps/prs/react/src/app/app.tsx | Adds AppHeaderMenu test items and navigation link |
| apps/prs/angular/src/routes/bugs/3450/bug3450.component.ts | Angular component for bug3450 test page |
| apps/prs/angular/src/routes/bugs/3450/bug3450.component.html | Angular template for bug3450 test page |
| apps/prs/angular/src/app/app.routes.ts | Adds routing for Angular bug3450 test page |
| apps/prs/angular/src/app/app.component.html | Adds AppHeaderMenu test items and navigation link (contains naming issue) |
| Super long menu item to test overflow handling lorem ipsum dolor sit amet | ||
| </a> | ||
| </goab-app-header-menu> | ||
| <goa-app-header-menu heading="John Smith" leadingIcon="person-circle"> |
There was a problem hiding this comment.
Inconsistent component name: This line uses 'goa-app-header-menu' instead of 'goab-app-header-menu'. All other instances in this file use the 'goab-' prefix. This should be changed to 'goab-app-header-menu' to match the consistent naming convention used throughout the file.
| _popoverEl.style.top = `${topPos}px`; | ||
| } else { | ||
| _popoverEl.style.bottom = "auto"; // In case this is triggered by _sectionHeight is changed | ||
| // Clear top/bottom to keep default below-target static positioning. |
There was a problem hiding this comment.
The comment mentions "static positioning" but the popover-content element has position: absolute (see line 551 in the style section). Consider updating the comment to clarify that clearing these properties allows the element to use its default absolute positioning relative to its offsetParent, which places it below the target.
| // Clear top/bottom to keep default below-target static positioning. | |
| // Clear top/bottom so the popover uses its default absolute positioning | |
| // relative to its offsetParent, which places it below the target. |
Before (the change)
The two issues were Dropdown not expanding outside a Container, and AppHeaderMenu not expanding to max width when necessary.
After (the change)
Both the above issues should be resolved.
Check samples in
It works for dropdowns in Containers:
It also works for inside a modal:
Also the main navbar was updated. Other than the border, the navbar items push right up to the side of the screen but don't go past:
Make sure that you've checked the boxes below before you submit the PR
Steps needed to test