Adopt platform-bible-react components in place of raw HTML (Tier 3) - #183
Conversation
alex-rawlings-yyc
added
the
up next
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesPlatform UI migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
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 |
![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.
bafd0c0 to
aad600d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
cc23481 to
67bb136
Compare
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 19 files and all commit messages.
Reviewable status: 19 of 24 files reviewed, 7 unresolved discussions (waiting on alex-rawlings-yyc).
|
I believe that the latest commit addresses all of the comments left from @imnasnainaec's most recent review |
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 5 files and all commit messages, made 1 comment, and resolved 6 discussions.
Reviewable status: 23 of 26 files reviewed, 3 unresolved discussions (waiting on alex-rawlings-yyc).
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 3 comments.
Reviewable status: 22 of 26 files reviewed, 2 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 4 files and all commit messages, and resolved 2 discussions.
Reviewable status: 25 of 26 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).
Modals now render through the platform Dialog, which brings a focus trap, scroll lock, focus restore, and Escape-to-dismiss; a modal mid-submission passes no onClose so Escape cannot abandon in-flight work. ViewOptionsDropdown drops its hand-rolled backdrop and getBoundingClientRect positioning for Popover. Both surfaces now report role="dialog", so e2e selects modals by [data-slot="dialog-content"]. Includes routine transitive lockfile bumps.
A dist/ left over from another branch ran an extension the tests were not written against, and the selector mismatches read as real regressions. Also scope modal lookups to a single dialog surface, since the discard-draft guard overlays the modal beneath it.
When the discard-draft guard stacks over a modal, the surface beneath it is inert, so the first-in-DOM match left leftover-modal cleanup unable to dismiss either one.
Escape or Cancel during the load closed the modal as if it had canceled, but the open completed and replaced the draft regardless.
Escape now collapses an armed delete or overwrite confirmation rather than closing the whole modal and discarding typed edits. The select modal's project rows go inert while a chosen project opens, and the Dialog test double routes Escape to the topmost dialog only, matching Radix's dismissal-layer stack.
Overriding it displaced the id Radix generates, so every modal open logged a spurious "requires a DialogTitle" error; e2e now locates modals by data-testid instead.
Gate outside-click on onClose the way Escape already is, and stop the read-only project-list loads from suppressing it — a fetch has nothing to abandon, so it left the picker inert for as long as the backend took.
Retire the load generation on unmount, and add the missing superseded guard to the select modal's catch, so a rejection that lands after the user hits Escape no longer fires an error notification.
The rebase dropped ModalShell's platform-bible-react import, so the branch did not type-check. The doc edits clear pre-rebase leftovers the comment rules bar.
Both modals loaded the same list with their own generation counter and disagreed on malformed responses; the hook cancels via effect cleanup and logs rather than notifies for a bad shape. Also notes that a caller may leave isOpening false, and refreshes stale modal-dismissal comments.
Both primitives carry the role, so ModalShell's modal-only assertions now select on the dialog-content slot instead.
5dfa0df to
88fbef5
Compare
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 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
2 participants
Resolves Tier 3 of #165.
ViewOptionsDropdownnow usesPopover, andModalShellusesDialog— bothwere hand-rolling overlays the platform already ships. The Dialog brings a focus
trap, scroll lock, and Escape-to-dismiss; modals mid-submission opt out of Escape
so in-flight work can't be abandoned. Drops ~115 lines of manual positioning and
three now-unused
tw:modal-*utilities.e2e modal lookups move to
[data-slot="dialog-content"], since both primitivesrender
role="dialog". Global setup now builds the extension so a staledist/can't masquerade as a regression.
This change is
Summary by CodeRabbit