fix(#2286): scroll-lock gap + scrim outlier in trade modals - #2537
Merged
Conversation
…l open The inventory in #2286 is substantially out of date, so this fixes what actually diverges rather than what was reported. MEASURED ACROSS ALL FIVE TRADE MODALS: z-index all five z-[9999] — CONVERGED (issue reported z-50, z-[60], z-[61]) portal all five createPortal — CONVERGED Escape all five — CONVERGED reduced motion all five — CONVERGED scroll lock 4 of 5 — FundingExplainerModal missing <-- fixed scrim 4x bg-black/80, 1 outlier — SendPositionNftModal <-- fixed focus trap ZERO of five — still open, see below Two fixes: * FundingExplainerModal had no `useLockBodyScroll`, so the page scrolled behind it while the other four locked. That is a real bug, not just inconsistency. * SendPositionNftModal's `bg-black/60 backdrop-blur-[2px]` is now bg-black/80, matching the other four. WHAT I DID NOT DO, and why. The issue asks for a shared <Modal> primitive and a migration of all five. I did not ship that, for two reasons that compound: 1. These five components have ZERO test coverage — `grep -rl <name> __tests__` returns nothing for every one of them. 2. PLAYGROUND.md's definition of done requires clicking through the changed flow in a browser, which I cannot do here. "Types don't prove UX" is its wording and it is right. Migrating TradeConfirmationModal and ClosePositionModal — the two that move money — blind, with no tests and no visual check, is not a trade worth making for a consistency refactor. The primitive is still the correct end state; it wants a browser. ALSO CORRECTING MY OWN MEASUREMENT: my first pass reported all five as missing scroll lock and three as having a focus trap. Both were grep artifacts — `useLockBodyScroll` did not match my pattern, and `focus=7` was counting Tailwind `focus:` classes. The numbers above are from matching the actual hook names. Launch suite: 3149 passed / 16 skipped / 0 failed. Refs: #2286 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the two divergences that actually remain in #2286, and reports the ones that have already closed themselves since the issue was filed.
The inventory in #2286 is out of date
Measured across all five trade modals on
origin/playground:z-[9999]z-50,z-[60],z-[61])createPortalFundingExplainerModalmissing — fixed herebg-black/80, 1 outlierSendPositionNftModal— fixed hereWhat this changes
FundingExplainerModalhad no scroll lock. The other four calluseLockBodyScroll(); this one did not, so the page scrolled behind it. That is a real bug rather than an inconsistency, and it is one line.SendPositionNftModal's scrim wasbg-black/60 backdrop-blur-[2px]againstbg-black/80everywhere else. Normalised.What this does NOT do, and why
The issue asks for a shared
<Modal>primitive with all five migrated onto it. I did not ship that. Two reasons that compound:TradeConfirmationModalandClosePositionModalare the two modals that move money. Migrating them with no tests and no visual check, for a consistency refactor, is not a good trade. The primitive is still the right end state — it wants a browser, and it is now a much smaller job than when the issue was filed, since z-index and portal have already converged on their own.Leaving #2286 open for the primitive + focus traps.
Correcting my own first measurement
My initial survey reported all five missing scroll lock and three having focus traps. Both were grep artifacts — my pattern did not match the
useLockBodyScrollhook name, and the focus count was picking up Tailwindfocus:classes. The table above matches on actual hook names.Verification
npx tsc --noEmitcleannpx vitest run— 3149 passed / 16 skipped / 0 failed🤖 Generated with Claude Code
https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D