Anchor the suggestion dropdown with Popover - #201
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesSuggestion popover migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
actor User
participant TokenChip
participant Popover
participant SuggestionDropdown
User->>TokenChip: Enter or navigate gloss suggestions
TokenChip->>Popover: Open anchored popover
Popover->>SuggestionDropdown: Render listbox content
SuggestionDropdown->>TokenChip: Report active option or selection
TokenChip->>User: Update ARIA state and retain input focus
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Drops the hand-rolled positioning, scroll/resize listeners, and close-on-scroll-out rule; the panel now tracks the gloss field wherever the view scrolls it.
44e4808 to
d172f38
Compare
The panel is the popover's own element, so an li sits outside any list and escapes Tailwind's ol/ul/menu list-style reset, growing a marker on every row. The div needs an explicit tabIndex the li's non-interactive implicit role was suppressing the check for.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed all commit messages.
Reviewable status: 0 of 4 files reviewed, 6 unresolved discussions (waiting on alex-rawlings-yyc).
Also makes the popover mock's close sentinel honor preventDefault, so the close-focus test can fail when the handler stops suppressing it.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 6 comments.
Reviewable status: 0 of 4 files reviewed, 6 unresolved discussions (waiting on alex-rawlings-yyc and imnasnainaec).
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 1 file and all commit messages, and resolved 4 discussions.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on alex-rawlings-yyc).
Swap the contradictory p-0/py-1 for px-0/py-1, name the assumption that keeps the anchor span as wide as the gloss field, and drop the scroll test's unread rect stub.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__mocks__/platform-bible-react.tsx (1)
839-841: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove the popover sentinels outside the listbox.
SuggestionDropdownsetsrole="listbox"onPopoverContent, then the mock wraps the test-onlypopover-closeandpopover-outsidebuttons inside that same element. Those buttons are no production options or outside-dismiss affordances, and they skew screen-reader/focus tests. Render them as siblings of the role-bearing content element or in a wrapper outside the role-bearing content itself.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__mocks__/platform-bible-react.tsx` around lines 839 - 841, Update the mock’s SuggestionDropdown rendering so the test-only popover-close and popover-outside sentinel buttons are siblings of, or wrapped outside, the PopoverContent element carrying role="listbox"; keep the actual suggestion content inside the listbox and preserve both sentinel buttons’ behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@__mocks__/platform-bible-react.tsx`:
- Around line 839-841: Update the mock’s SuggestionDropdown rendering so the
test-only popover-close and popover-outside sentinel buttons are siblings of, or
wrapped outside, the PopoverContent element carrying role="listbox"; keep the
actual suggestion content inside the listbox and preserve both sentinel buttons’
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 32a24a7a-9a5e-426c-8d4a-94e8a3f52251
📒 Files selected for processing (3)
__mocks__/platform-bible-react.tsxsrc/__tests__/components/TokenChip.suggestions.test.tsxsrc/components/SuggestionDropdown.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/SuggestionDropdown.tsx
- src/tests/components/TokenChip.suggestions.test.tsx
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 4 files and all commit messages, and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
2 participants
This change is
Summary by CodeRabbit
Accessibility
Bug Fixes