Scale back to the add button's margin centering - #259
Conversation
The platform button's pressed style is a translate, which overwrites the same custom property a translate utility uses for centering — so the suggestion "+" and the arc split button both jumped mid-click.
The platform Button no longer clobbers a caller's centering translate: paranext/paranext-core#2667 moved its press nudge off the shared `--tw-translate-y` custom property onto `transform`, and the rebuilt `platform-bible-react` carrying that fix is on `paranext-core` main. Translate-centering a platform Button is therefore correct again, so the arc split button and its test go back to what `main` had. Kept only the part that stands on its own: centering the suggestion "+" by margin inside an `inset-y-0` box, which keeps centering out of the transform channel the Button transitions and writes to on press. Also dropped the `not.toMatch(/translate/)` assertions. Their premise — that a translate utility on a platform Button is unsafe — is no longer true, and the invariant now lives upstream in `button.test.tsx`, where it covers every caller rather than these two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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 (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe ChangesTokenChip suggestion centering
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This is a localized change to center the suggestion button by margin and update its test coverage; no actionable merge-blocking risk remains after normal checks and review. 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 |
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).
Scales #214 back to the one change in it that stands on its own, now that the upstream fix it was working around has landed and been built.
paranext/paranext-core#2667 moved the platform
Button's press nudge off the shared--tw-translate-ycustom property ontotransform. Verified it is consumable, not just merged:ff22ea69aa9is an ancestor ofparanext-coremainlib/platform-bible-react/distwas rebuilt after it (238ace491d3,779ab347d27,35a09c06276), and the live class in both the ESM and CJS bundles is nowtw:active:not-aria-[haspopup]:transform-[translateY(1px)]platform-bible-reactas a lockfile link to../paranext-core/lib/platform-bible-react, so there is no version to bump — pullingparanext-coremainis enoughThe browser applies
translatebeforetransform, so a caller's-translate-x-1/2 -translate-y-1/2composes with the nudge instead of replacing it. Translate-centering a platformButtonis correct again.What's kept
Centering the suggestion "+" by margin inside an
inset-y-0box, inTokenChip.tsx, plus a test for it. This one earns its place independent of the bug it was written for:Buttonvariant, animation utility, or anything else that writestranslate— #2667 only moved the current nudge, and theButtonstill carriestw:transition-allWhat's dropped
ArcOverlay.tsx) and its test, back to whatmainhad. Keep icon buttons centered while pressed #214 centered it with an inlinestyletransform, which #2667's own note lists among the things that still override the nudge — so that button would have silently lost its press feedback.not.toMatch(/translate/)assertions. Their premise — that a translate utility on a platformButtonis unsafe — is no longer true, and the assertion would forbid an approach that is now fine. The invariant lives upstream inbutton.test.tsx, where it covers every caller rather than these two.Test plan
npx jest— 63 suites, 1785 tests passnpm run lint— eslint, stylelint,tsc --noEmit, dependency scope all cleantw:relativespan, and the button has a definiteh-2.5), but a visual check of the "+" against the gloss input border is worth doing before this comes out of draft.Follow-ups
Branched from
202-suggestion-add-button-jumps(rebased ontomain) so @alex-rawlings-yyc's commit keeps its attribution; the scale-back is the commit on top.🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
Style
Tests