fix(input_number): give the tall tier's stepper way to the vertical slider - #313
Conversation
…lider A `tall` tile is one column wide by definition, which on a 12-column desktop grid is 63px with a 35px content region. The stepper is a content-sized row — two 40px icon buttons, a 60px value button and two 8px gaps, 156px in all — and Radix's `.rt-BaseButton` sets `flex-shrink: 0`, so it could not compress into that region: it overhung 46.5px past each of the tile's own edges and `.liebe-card`'s `overflow: hidden` cut the buttons off. Change 0042 PR 1 settled the contract this applies: a control bounded by its contents on every axis cannot be made cross-axis flexible, so it is omitted at that tier in favour of one that can — here the vertical slider the tier's layout already asks for. The resolution is presentation only, the shape `resolveSelectPresentation` already had: the stored `controlStyle` is never rewritten, `row` and `full` resolve exactly as before, and the card returns to the stepper as soon as it is two columns wide again. `--liebe-card-padding` is untouched. The inset is shared, so buying room here would move the vertical slider too — that is PR 3's, and under LCARS it would move the wrong way. The e2e assertion measures every rendered control against the tile's border box, which is what `overflow: hidden` crops against. The slider's own overhang inside the padding is a separate claim at a separate tolerance and belongs to PR 3; asserting it here would fail on code that is correct for this task. Refs: docs/changes/0042-tall-tile-control-geometry.md (PR 2)
The line asked for the control measured against the tile's CONTENT box while the same document's testing requirements split the two consequences on exactly that distinction — the stepper's is a clip past the tile's edge, the vertical slider's is an overhang inside the padding — and options/input-helpers' own scenario for this fallback asks for the edge. Taken literally the line would have gated this task on PR 3's work, since a 42px slider in a 35px content region is correct until PR 3 makes the track flexible.
…ldren The slider is a direct child of `.liebe-card-controls` today, so the union is the same set — but a wrapper introduced later would be constrained to the slot and would measure as a perfect fit around a track that escaped, which is the one failure this spec exists to catch.
The tier is stamped as soon as the card renders, which is before the helper's state has arrived over the websocket — a snapshot taken between the two would find an empty control slot and pass every fit comparison vacuously. The predicate is "any control" rather than "the vertical slider" on purpose: a poll for the slider would turn a stepper rendering here, the exact defect, into a timeout with no measurement in the failure.
"Held" reads as the whole tier being met, which the next sentence then takes back — the slider's own cross-axis fit is 0042 PR 3's.
The requirement asked for the control measured against the tile's CONTENT box whatever the fix, which contradicts the bullet two below it: the stepper's consequence is a clip past the tile's edge and the slider's is an overhang inside the padding, and only the second is a content-region claim. Taken as written it gated PR 2 on PR 3's work, since a 42px track in a 35px region is the state PR 2 is correct in. Also folds PR 2's task line back to a reference rather than a second copy of the reasoning.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesTall input control behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes input_number tiles configured with controlStyle: 'stepper' clipping their embedded stepper at the tall tier by resolving the rendered presentation to the tier’s vertical slider (without rewriting stored config). It implements PR 2 of docs/changes/0042-tall-tile-control-geometry.md.
Changes:
- Add a render-time resolver (
resolveNumberPresentation) sotallsubstitutes the vertical slider whenstepperis stored. - Update
InputNumberCardto use the resolved presentation and add unit + e2e coverage for the “no clipping past tile edge at tall” requirement. - Update related specs, change doc tasks, and Storybook to reflect the new
tallbehavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/tall-stepper-fit.spec.ts | New Playwright spec measuring control bounding boxes vs the tile border box at tall. |
| src/store/inputHelperOptions.ts | Adds resolveNumberPresentation to substitute slider at tall when stepper is stored. |
| src/store/tests/inputHelperOptions.test.ts | Unit tests for resolveNumberPresentation. |
| src/components/InputNumberCard.tsx | Uses resolveNumberPresentation for the embedded control rendering. |
| src/components/InputNumberCard.stories.tsx | Updates TierTall description and adds a story covering stored-stepper/rendered-slider at tall. |
| src/components/tests/inputHelperControlStyle.test.tsx | Card-level tests asserting tall renders vertical slider even when stepper is stored, and config is not rewritten. |
| src/components/tests/controlCardTierLayouts.test.tsx | Updates stale tier-table quote and aligns test commentary with the new rule. |
| src/components/tests/cardTierLayouts.test.tsx | Adds/adjusts tier layout assertions for the tall substitution behavior. |
| docs/specs/entity-cards/options/input-helpers.md | Updates status prose and documents the tall stepper→slider rule as landed by 0042 PR 2. |
| docs/specs/design-system/index.md | Updates cross-axis-fit section status to reflect PR 2 closure (while leaving PR 3/4 work open). |
| docs/changes/0042-tall-tile-control-geometry.md | Updates testing requirement wording and checks off PR 2 task with clarified measurement target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #313 +/- ##
==========================================
+ Coverage 87.60% 87.65% +0.04%
==========================================
Files 230 231 +1
Lines 8440 8472 +32
Branches 2848 2847 -1
==========================================
+ Hits 7394 7426 +32
Misses 823 823
Partials 223 223 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…at it is The doc block called the slider "cross-axis flexible", which the code does not have: its vertical orientation is still a fixed inline-size of --liebe-control-height in anatomy.css, and making it take the content region is 0042 PR 3's. Worse than an ordinary stale comment here, because a reader who believed the slider was already flexible would read the tier check as a redundant optimisation and delete it — when keying on the tier rather than on a measured width is the whole reason this survives a content region of zero.
Summary
An
input_numbercard set tocontrolStyle: 'stepper'now renders the tier's vertical slider attallinstead of a stepper the tile clips.rowandfullare untouched, and the stored option is never rewritten — the card returns to the stepper the moment it is two columns wide again.Implements PR 2 only of docs/changes/0042-tall-tile-control-geometry.md. PRs 3 and 4 remain open, so the change document stays
draftanddocs/index.yml/docs/index.mdare untouched (status: completestill counts 34).Why the stepper cannot stay
A
talltile is one column wide by definition — 63px on a 12-column desktop grid, leaving a 35px content region. The stepper is content-sized: two 40pxsize="3"icon buttons, the value button's 60px minimum and the control row's two 8px gaps, 156px in all. Radix's.rt-BaseButtonsetsflex-shrink: 0, so it could not compress into that region: it overhung 46.5px past each of the tile's own edges and.liebe-card'soverflow: hiddencut the buttons off. Editing the value made it worse — the readout becomes aTextFieldwith an inlinewidth: 80px, growing the row to 176px.This PR applies what PR 1 settled rather than deciding it: a control bounded by its contents on every axis has no floor to size down to, so it is omitted at that tier in favour of one that can — here the vertical slider the tier's layout already asks for, which keeps the entity operable. The mechanism is the shape
resolveSelectPresentationalready had forinput_select's pills.The content region has no lower bound, and the fix survives that. A screen stored at 16 columns is honoured at both desktop breakpoints, so a 960px container lays out a 43px tile and LCARS's 44px inline inset leaves no content region at all. Nothing here is expressed as "there is enough room" — the substitution is keyed on the tier, which is exactly the condition (one column wide), so it holds at every width including zero. Making the slider that replaces it flexible down to the 24px floor, and omitted below it, is PR 3's.
--liebe-card-paddingis not touched. The inset is shared, so buying room here would move the vertical slider too, and under LCARS it would move the wrong way. PR 1's decision removes the temptation rather than relying on the warning.Changes
resolveNumberPresentation(style, tier)insrc/store/inputHelperOptions.ts— presentation resolved at render, config never rewritten, sitting beside itsinput_selectsibling.src/components/InputNumberCard.tsxconsumes it for the embedded control; the detail dialog's control is unaffected (it resolves from the helper'smodeand is never at this tier).controlCardTierLayouts.test.tsx— it still quoted "(or compact stepper)", the phrase PR 1 dropped rather than defined.TierTallstory's comment, which said "stepper between" for a fixture that has rendered a slider since 0022.Two documentation defects found on the way
Both were the change document contradicting itself about which box PR 2 measures against, and both are fixed here rather than worked around:
Caught by Codex and CodeRabbit independently.
Testing
resolveNumberPresentationacross all four tiers and both stored styles; card-level coverage thattallrenders the vertical slider with none of the stepper's three surfaces present, thatrowandfullstill render the stepper from the same stored config, and that the config object is unchanged after a tier that could not render one.tests/e2e/tall-stepper-fit.spec.tsseeds a 1×3input_numberwithcontrolStyle: 'stepper'set explicitly and measures every rendered control against the tile's border box, which is whatoverflow: hiddencrops. The assumed grid-container width is recorded, and the spec asserts the tile is narrower than the stepper's 156px so a later viewport change cannot quietly turn it into a no-op. Controls are measured individually rather than through their slot — the slot is constrained to the tile and would have reported a perfect fit for exactly this defect. Not run locally; CI'sHome Assistant E2Ejob is the gate.row→ 8 failed.Gates
npm testnpm run lintMigrationCutoffswarningnpm run typechecknpm run test:coveragenpm run build-storybookcodex review --base maincr)skipped (rate-limited)All run unpiped with their real exit status read, after merging
origin/main(which brought in theiconOnlyoption — no interaction).Summary by CodeRabbit
New Features
input_numbercards now display a vertical slider instead of stepper controls, even when a stepper is configured.Bug Fixes
Documentation