fix: convert to km in locator component #1026
Conversation
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
WalkthroughDistance handling across the visual-editor was refactored to be unit- and locale-aware. New distance utilities (toMeters, fromMeters, toMiles, formatDistance, getPreferredDistanceUnit) were added and distances are stored and computed in meters internally. Components, state, and public props were renamed or updated from miles-centric (selectedDistanceMiles, DEFAULT_RADIUS_MILES) to meters-centric (selectedDistanceMeters, DEFAULT_RADIUS) and DistanceFilter now emits both distance and unit. UI rendering, filters, and API calls format and display distances using the i18n-preferred unit while using meters for computations and API radius values. Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Locator / DistanceFilter
participant I18n as i18n / getPreferredDistanceUnit
participant Utils as distance utils
participant API as Backend / Search
User->>UI: open distance picker
UI->>I18n: getPreferredDistanceUnit(language)
I18n-->>UI: preferredUnit
UI->>Utils: formatDistance(fromMeters(optionMeters, preferredUnit), language)
Utils-->>UI: localizedLabel
User->>UI: select option (distanceValue, preferredUnit)
UI->>Utils: toMeters(distanceValue, preferredUnit)
Utils-->>UI: distanceMeters
UI->>API: emit onChange(distanceMeters, preferredUnit) / update filters (meters)
API-->>UI: results (distances in meters)
UI->>Utils: formatDistance(fromMeters(resultMeters, preferredUnit), language)
Utils-->>UI: displayDistance
UI-->>User: render displayDistance
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
1539-1558:⚠️ Potential issue | 🟠 MajorAccessibility bug: aria-label not updated to use localized distance unit.
The aria-label on line 1542 still uses hardcoded
t("mile", ...)while the display text (lines 1553-1558) correctly shows kilometers for non-imperial locales. Screen reader users will hear "miles" while visual users see "kilometers".Additionally,
toKilometers(distanceMiles)is computed twice — consider computing it once for clarity and consistency.🐛 Proposed fix
{distanceOptionsMiles.map((distanceMiles) => ( <div className="flex flex-row gap-4 items-center" id={`distanceOption-${distanceMiles}`} key={distanceMiles} > + {(() => { + const displayDistance = + unit === "mile" ? distanceMiles : toKilometers(distanceMiles); + return ( + <> <button className="inline-flex bg-white" onClick={() => onChange(distanceMiles)} - aria-label={`${t("selectDistanceLessThan", "Select distance less than")} ${distanceMiles} ${t("mile", { count: distanceMiles })}`} + aria-label={`${t("selectDistanceLessThan", "Select distance less than")} ${displayDistance} ${t(unit, { count: displayDistance })}`} > <div className="text-palette-primary-dark"> {selectedDistanceMiles === distanceMiles ? ( <FaDotCircle /> ) : ( <FaRegCircle /> )} </div> </button> <Body className="inline-flex"> - {`< ${ - unit === "mile" ? distanceMiles : toKilometers(distanceMiles) - } ${t(unit, { - count: - unit === "mile" ? distanceMiles : toKilometers(distanceMiles), - })}`} + {`< ${displayDistance} ${t(unit, { count: displayDistance })}`} </Body> + </> + ); + })()} </div> ))}Alternatively, compute
displayDistanceat the start of the map callback:{distanceOptionsMiles.map((distanceMiles) => { const displayDistance = unit === "mile" ? distanceMiles : toKilometers(distanceMiles); return ( <div ...> <button aria-label={`... ${displayDistance} ${t(unit, { count: displayDistance })}`}> ... </button> <Body>{`< ${displayDistance} ${t(unit, { count: displayDistance })}`}</Body> </div> ); })}
🤖 Fix all issues with AI agents
In `@packages/visual-editor/src/components/LocatorResultCard.tsx`:
- Around line 602-609: The distance is computed separately for miles and
kilometers and formatted inconsistently (Number(toFixed(1)) and
t("distanceInUnit")), causing breakpoints to show different units and losing
locale formatting/ trailing zeros; replace distanceInMiles and
distanceInKilometers with a single derived displayDistance and unitLabel based
on the existing unit prop/variable, compute the numericValue by converting
distance (meters) to the selected unit, then format it once with
Intl.NumberFormat(undefined, { minimumFractionDigits: 1, maximumFractionDigits:
1 }) to preserve trailing zeros and locale rules, and use that displayDistance +
unitLabel everywhere (including where t("distanceInUnit", …) was used) so
desktop badge and mobile share the exact same formatted string; update usages in
the code that reference distanceInMiles, distanceInKilometers, and
t("distanceInUnit") to the new displayDistance/unitLabel.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
1512-1561:⚠️ Potential issue | 🟠 MajorFix selection state + aria label when unit is km.
When unit is kilometers,
selectedDistanceMiles(miles) is compared todistanceOption(km), so the selected indicator never lights up. The aria-label also hardcodes “mile”. Convert once per option and reuse for selection, onChange, and labels.🛠️ Suggested fix
- <div {...getCollapseProps()}> - {distanceOptions.map((distanceOption) => ( - <div - className="flex flex-row gap-4 items-center" - id={`distanceOption-${distanceOption}`} - key={distanceOption} - > - <button - className="inline-flex bg-white" - onClick={() => - onChange( - // onChange treats the underlying unit as miles. If we're viewing in km, convert back to miles - unit === "mile" ? distanceOption : toMiles(distanceOption) - ) - } - aria-label={`${t("selectDistanceLessThan", "Select distance less than")} ${distanceOption} ${t("mile", { count: distanceOption })}`} - > - <div className="text-palette-primary-dark"> - {selectedDistanceMiles === distanceOption ? ( - <FaDotCircle /> - ) : ( - <FaRegCircle /> - )} - </div> - </button> - <Body className="inline-flex"> - {`< ${distanceOption} ${t(unit, { - count: distanceOption, - })}`} - </Body> - </div> - ))} - </div> + <div {...getCollapseProps()}> + {distanceOptions.map((distanceOption) => { + const distanceOptionMiles = + unit === "mile" ? distanceOption : toMiles(distanceOption); + const isSelected = selectedDistanceMiles === distanceOptionMiles; + return ( + <div + className="flex flex-row gap-4 items-center" + id={`distanceOption-${distanceOption}`} + key={distanceOption} + > + <button + className="inline-flex bg-white" + onClick={() => onChange(distanceOptionMiles)} + aria-label={`${t("selectDistanceLessThan", "Select distance less than")} ${distanceOption} ${t(unit, { count: distanceOption })}`} + > + <div className="text-palette-primary-dark"> + {isSelected ? <FaDotCircle /> : <FaRegCircle />} + </div> + </button> + <Body className="inline-flex"> + {`< ${distanceOption} ${t(unit, { count: distanceOption })}`} + </Body> + </div> + ); + })} + </div>
🤖 Fix all issues with AI agents
In `@packages/visual-editor/src/components/LocatorResultCard.tsx`:
- Around line 605-617: displayDistance is only assigned when distanceInMiles and
distanceInKilometers are truthy, so a zero distance is skipped; change the
checks to numeric (typeof === "number") so 0 is allowed. Specifically, keep
computing distanceInKilometers and distanceInMiles, then set displayDistance
when either converted value is a number (use typeof checks) and choose the value
based on getPreferredDistanceUnit(i18n.language), formatting with
formatDistance(i18n.language) and appending the unitLabel; update the
conditional around displayDistance and the unit selection logic (references:
distanceInKilometers, distanceInMiles, displayDistance,
getPreferredDistanceUnit, formatDistance, i18n.language).
In `@packages/visual-editor/src/utils/i18n/distance.ts`:
- Around line 13-15: toKilometers currently returns full precision which can
leak long decimals into UI; restore backward-compatible rounding by adding an
optional parameter (e.g., decimals?: number) to toKilometers and return the
value rounded to that many decimals (default to the previous behavior, e.g., 2),
or alternatively ensure all callers use formatDistance—update the toKilometers
function signature and implementation (function name: toKilometers) and adjust
any call sites that relied on the prior rounded output.
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/visual-editor/src/components/Locator.tsx`:
- Around line 611-612: The locale check in getPreferredDistanceUnit currently
treats only ["en","en-GB"] as imperial and misses other English imperial
locales; update IMPERIAL_DISTANCE_LOCALES to include en-US, en-AU, en-NZ, en-CA
(and any other imperial English variants you want supported) OR refactor
getPreferredDistanceUnit to detect i18n.language by prefix (startsWith("en"))
and then exclude known metric English locales (e.g., "en-GB" stays imperial but
exclude any metric exceptions) so preferredUnit derived from
useTranslation().i18n.language correctly returns imperial units for all English
variants that use them.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
840-991:⚠️ Potential issue | 🟡 MinorAdd
preferredUnitto the dependency array of theresolveLocationAndSearcheffect.The effect uses
preferredUnitat lines 845, 1044, and 1063 to calculate the default search radius, butpreferredUnitis not included in the dependency array at line 991. This means if the user's locale changes (and thuspreferredUnitchanges viai18n.language), the effect won't re-run to recalculate the radius with the updated unit.A similar effect in the same component (lines 782–783) correctly includes
preferredUnitin its dependency array, suggesting this omission is an oversight rather than intentional.Update the dependency array to:
[searchActions, mapStartingLocation, initialLocationParam, preferredUnit]
🧹 Nitpick comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
53-58: Unused imports detected.
formatDistanceandfromMetersare imported but do not appear to be used in this file. Consider removing them to keep the imports clean.♻️ Proposed fix
import { - formatDistance, - fromMeters, getPreferredDistanceUnit, toMeters, toMiles, } from "../utils/i18n/distance.ts";
...es/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocationsCardsWrapper.tsx
Show resolved
Hide resolved
vijay267
left a comment
vijay267
left a comment
There was a problem hiding this comment.
Overall looks good. Just had one question about getPreferredDistanceUnit. Thanks!
mkilpatrick
dismissed stale reviews from vijay267, mkouzel-yext, jwartofsky-yext, asanehisa, and briantstephan
via
886e15e
6 participants
No description provided.