feature: metric imperial choice by cimigree · Pull Request #1867 · digidem/comapeo-mobile

cimigree · 2026-04-27T22:53:14Z

closes #1818

Adds a property in settings for someone to select which unit they would like to use, metric or imperial.
Adds the screen where they make that selection
Adds the SVG for the icon for the line in settings
Adds an E2E test for changing the value
Adds this to context and providers and defaults to metric.
Adds a utility function and a test to create that conversion by passing it what needs to be converted and the unit system and for one case, the decimals to use. For the one that uses decimals, there was only one place where we want to fixed(2) -- the observation tooltip, so I added that there. Is it definitely necessary?
I used that converter in all of the places I could find that have distances.
I updated some of the old Text components
See attached screen shots.
Did I miss any?

…l for conversion. Uses the conversion in some places.

ErikSin

Overall looks good, there are just 3 small but blocking comments:

The functions should not be dealing with the decimal conversions
I don't think m/s should be converted to mph
using 0 in your functions to get the unit is awkward.

ErikSin · 2026-04-30T17:05:18Z

+export const UnitSystemStoreContext = createContext<UnitSystemStore | null>(
+  null,
+);
+export const UnitSystemStoreProvider = UnitSystemStoreContext.Provider;


Provider will be deprecated in the next release, so lets start NOT using it.

ErikSin · 2026-04-30T17:24:30Z

+export function metersPerSecondOrConversion(
+  mps: number,
+  unitSystem: UnitSystem,
+): {value: string; unit: string} {
+  if (unitSystem === 'imperial') {
+    return {value: (mps * MS_TO_MPH).toFixed(2), unit: 'mph'};
+  }
+  return {value: mps.toFixed(2), unit: 'm/s'};
+}


Why are we converting meter per second into miles per hour? In terms of scale, meters and miles are quite different. Wouldn't ft/s be the more appropriate conversion here?

ErikSin · 2026-04-30T17:59:31Z

+  meters: number,
+  unitSystem: UnitSystem,
+  decimals = 0,
+): {value: string; unit: string} {


We should strongly type the unit for all of these since string is too general. Currently there is no way of typescript to know whether it is feet, meters, mph, mps....etc since it can be ANY string.

type Unit = "ft" | "m" export function metersOrConversion(...params): {value: string; unit: Unit}(....rest)

ErikSin · 2026-04-30T18:25:03Z

+      <SelectOne
+        value={unitSystem}
+        options={options}
+        onChange={selected => {
+          setUnitSystem(selected);
+        }}
+      />


Can you actually get rid of this SelectOne in this file?. This was migrated over from Mapeo, and its an unnecessary abstraction. I think we should just copy and paste the code from Select One twice - one for each unit.

ErikSin · 2026-05-06T20:01:33Z

+export function kmOrConversion(
+  km: number,
+  unitSystem: UnitSystem,
+): {value: string; unit: string} {


same comment about strongly typing unit here

ErikSin · 2026-05-06T20:01:37Z

+export function metersPerSecondOrConversion(
+  mps: number,
+  unitSystem: UnitSystem,
+): {value: string; unit: string} {


same comment about strongly typing unit here

ErikSin · 2026-05-06T20:11:14Z

I think all these functions should NOT deal with the decimal places. the amount of decimal places can be dealt with directly with the consuming components. I know it makes the code a little more verbose, but simpler functions that do less things are better for testing purposes and avoids subtle bugs down the line

ErikSin · 2026-05-06T20:17:24Z

+
  const manualLocation = metadata?.manualLocation;
+  const lengthUnit = metersOrConversion(0, unitSystem).unit;
+  const speedUnit = metersPerSecondOrConversion(0, unitSystem).unit;


this is a bit akward, calling the same function with the dummy value 0.

Maybe just create 2 helper functions in unitConversion.ts

export function getMetersUnit(unitSystem: UnitSystem): Unit { return unitSystem === 'imperial' ? 'ft' : 'm'; } export function getSpeedUnit(unitSystem: UnitSystem): Unit{ return unitSystem === 'imperial' ? 'mph' : 'm/s'; }

ErikSin · 2026-05-06T20:20:51Z

+                      ? (() => {
+                          const {value, unit} = kmOrConversion(
+                            warningInfo.distanceKm ?? 0,
+                            unitSystem,
+                          );
+                          return t(m.distanceAway, {
+                            distance: `${value} ${unit}`,
+                          });
+                        })()


This is a style thing, so I am not asking you to change it. But I personally think IIFE in react are really hard to read. Perhaps something we should discuss next week when we are together to determine if there are certain style things that should/can be avoided.

I agree.
Easier to read when not IIFE, for the most part. I left 1 in (in LocationView) because it was very simple and prevented extra function calls.
7e1c3cb

…cond now. No select one component. Replaces context.provider. Helpers for getting units. Call sites format numbers.

…its.

cimigree · 2026-05-07T18:40:35Z

I had to make a guess about how the Unit should be formatted on the new Settings screen. Following the previous design choices made sense to me so that is what I did, showing either imperial or metric based on what they have. Should I confirm with design? It just seemed to follow logically...

Also, I followed what was being done with internationalization by only using it in the one place it was already being used, I think, in AttachedPhotoPreviewModal. In all the other places I had to add a unit conversion, Internationalization is not/ was not being used, so I did not add it because there are so many diffs in this file already and I wasn't sure it was desired behavior. However, I am wondering if we should have a ticket to internationalize all of the numbers?

ErikSin

2 non blocking comments. Overall looking good, thanks for doing that

ErikSin · 2026-05-19T18:16:07Z

+export function UnitSystemStoreProvider({
+  value,
+  children,
+}: {
+  value: UnitSystemStore;
+  children: React.ReactNode;
+}) {
+  return React.createElement(UnitSystemStoreContext, {value}, children);
+}


We shouldn't be using createElement here. In general we should either use a jsx component instead of trying to force an react element inside of a non jsx component.

I think the better thing to do here is to NOT export a Provider and just use the Context directlty in AppProviders.tsx

eg:

//In AppProvider.tsx return ( <UnitSystemStoreContext value={unitSystemStore}> ...rest of the providers </UnitSystemStoreContext> )

ErikSin · 2026-05-19T19:04:15Z

+function formatDistance(
+  meters: number,
+  unitSystem: UnitSystem,
+  formatNumber: (value: number, opts: object) => string,


This relates to your comment about intl. I think we either should use formatNumber for everything or not at all. Using it only for this component is a little confusing, expecially since we are dealing with the formatting manually in all the other components. I would argue to not use formatNumber here and just do it manually like the rest of the components

dbb984c

Should we have a ticket to update all numbers to use intl?

i don't know. Do you think we should use intl? The manual formatting seems fine to me, but im not sure i fully understand the benefits of using intl

I think it is some differences of commas and periods? Not a huge difference but somewhat of a difference. https://www.smartick.com/blog/other-contents/curiosities/decimal-separators/
German, French, Spanish: 1.234,56 (period as thousands separator, comma as decimal)
English: 1,234.56 (comma as thousands separator, period as decimal)
Maybe a low priority ticket? Find out what others think? I don't have strong opinions about it.

…out intl number formatting since it is only used in one place.

cimigree added 6 commits April 27, 2026 12:54

Adds unit conversion choice UI. Adds context and provider. Adds a uti…

cc04704

…l for conversion. Uses the conversion in some places.

Gets rid of deprecated text component.

c3294b6

Adds more conversions as needed.

afe57a6

More uses of distance that need to be converted.

d03287f

Adds unit system store to helper for test.

0e0363f

Enhances E2E test to account for checking for imperial or metric.

bf40cc5

cimigree requested a review from ErikSin April 28, 2026 15:05

ErikSin requested changes May 6, 2026

View reviewed changes

cimigree added 3 commits May 6, 2026 17:41

Fixes merge conflicts and merges with develop

0b4166f

Strong unit types. Conversions return raw numbers. Speed is ft per se…

62701f6

…cond now. No select one component. Replaces context.provider. Helpers for getting units. Call sites format numbers.

Removes IIFE's. Fixes E2E test. Makes sure to use correct-matching un…

7e1c3cb

…its.

cimigree requested a review from ErikSin May 7, 2026 18:41

ErikSin approved these changes May 19, 2026

View reviewed changes

Just exports context and not provider to use in App Providers. Takes …

dbb984c

…out intl number formatting since it is only used in one place.

cimigree merged commit 96209d6 into develop May 20, 2026
11 checks passed

cimigree deleted the feature/metric-imperial-choice branch May 20, 2026 19:15

Uh oh!

Conversation

cimigree commented Apr 27, 2026

Uh oh!

ErikSin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cimigree commented May 7, 2026

Uh oh!

ErikSin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants