fix(editor): unit-aware sidebar controls + preserve user-typed precision#238
Open
b9llach wants to merge 1 commit intopascalorg:mainfrom
Open
fix(editor): unit-aware sidebar controls + preserve user-typed precision#238b9llach wants to merge 1 commit intopascalorg:mainfrom
b9llach wants to merge 1 commit intopascalorg:mainfrom
Conversation
Two related fixes to the sidebar's length inputs:
1. `SliderControl` now reads `useViewer.unit` and, when the caller
passes `unit="m"`, displays / edits / round-trips length values in
feet when the user has toggled imperial. Scene data stays in
metres — conversion is display-only. Roughly 70 slider instances
across every panel pick up imperial support for free with no
call-site changes. Fixes a pre-existing half-wired feature: the
`useViewer.unit` toggle in the toolbar was only respected by
`wall-measurement-label`, `site-edge-labels`, `metric-control`,
and `floorplan-panel`; every panel slider hardcoded `"m"` as its
suffix and showed raw metres regardless.
2. Removed parent-side `Math.round(node.X * 100) / 100` clipping
from every panel that passed it as the `value` prop to a slider.
These defensive rounds were there to avoid floating-point display
noise but actively broke the new imperial round-trip: typing
`8.00 ft` into a door-height slider with `precision={2}` would
round the converted metres value to `2.44 m` (losing 4 mm), then
the re-display `2.44 × 3.28084 ≈ 8.0052` rounds to `"8.01"`.
SliderControl's own `.toFixed(precision)` already handles the
display cleanliness the parents were trying to achieve, so the
parent-side rounds were redundant even in metric mode.
The `submitValue` path also now rounds in the DISPLAY unit before
converting back to metres, so user-typed values preserve their
exact precision regardless of the stored unit.
New: `packages/editor/src/lib/units.ts` — shared `formatLength`,
`formatArea`, `metersToFeet`, `feetToMeters` helpers that replace
the four ad-hoc copies scattered around the editor. The slab and
ceiling panel Info sections now use `formatArea(area, unit)`
instead of a hardcoded `"m²"` suffix.
Caveats:
- Sliders with non-`"m"` units (percent, angle degrees, etc.) are
unchanged — only `unit="m"` triggers the imperial conversion.
- Follow-up opportunity: the four existing `formatMeasurement` /
`formatArea` duplicates in `wall-measurement-label.tsx`,
`site-edge-labels.tsx`, `metric-control.tsx`, and `floorplan-
panel.tsx` could be consolidated onto the new `lib/units.ts`
helpers in a separate refactor PR.
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
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.
What does this PR do?
Two related fixes to the sidebar's length inputs.
1.
SliderControlis unit-awareuseViewer.unit('metric' | 'imperial') already existed and was toggled by the toolbar m/ft button, but the only places that actually respected it werewall-measurement-label,site-edge-labels,metric-control, and the 2Dfloorplan-panel. Every sidebar slider hard-codedunit="m"as its suffix and showed raw metres regardless of the user's preference. This PR teachesSliderControlto readuseViewer.unitand, when the caller passesunit="m", convert the value for display + editing while keeping scene data in metres (conversion is display-only). About 70 slider instances across every panel pick up imperial support with zero call-site changes.Sliders with non-
"m"unit strings (e.g."%","°") are unchanged.2. Parent-side
Math.round(...)clipping removed from panelsEvery panel had a
value={Math.round(node.X * 100) / 100}pattern around its sliders. It was there to defend against floating-point display noise (2.10000000000000001→"2.10"), butSliderControl's own.toFixed(precision)already does that — so the parent rounds were redundant even in metric mode.With the imperial path landed, the parent rounds became actively harmful. Tracing the door-height
precision={2}case:"8.01 ft"(whatever was stored), types8.00, EntersubmitValueconverts:8 / 3.28084 ≈ 2.43840 monChange(2.43840)— handler callsupdateNode({ height: 2.43840 })door-panel.tsxpassesvalue={Math.round(node.height * 100) / 100}=2.44SliderControlgets2.44, displaystoDisplay(2.44).toFixed(2)=(2.44 × 3.28084).toFixed(2)="8.01"So typing
8.00bounced back to8.01. Removing theMath.roundin the parent lets the full-precision metres value survive the round trip, and the display cleanly shows"8.00"again.The
submitValuepath also now rounds in the display unit before converting back to metres, so user-typed values preserve their exact precision regardless of which unit is stored underneath.New shared helper
packages/editor/src/lib/units.ts—formatLength,formatArea,metersToFeet,feetToMeters,METERS_TO_FEET. The slab + ceiling Info sections now useformatArea(area, unit)instead of a hardcoded"m²"suffix. Follow-up opportunity (out of scope for this PR): consolidate the four existing ad-hoc copies inwall-measurement-label.tsx,site-edge-labels.tsx,metric-control.tsx, andfloorplan-panel.tsxonto this new module.How to test
bun devmtoft, and the numbers should reflect the conversion (2.1 m height shows as 6.89 ft).8.00into any length slider while in imperial mode, press Enter: value should stay at8.00 ft(onmainit snaps to8.01 ft)."12.50 m²"in metric and"134.55 ft²"in imperial.Screenshots / screen recording
N/A — unit-toggle behaviour, hard to screenshot without video. Will record a short clip if reviewers want one.
Checklist
bun devbun checkpasses on the touched files — verified viabiome checkat@biomejs/biome@^2.4.6, which is the version pinned in rootpackage.json)mainbranch