Imperial Units Feature

[ThomasLatham]

✅ I have read the contributing file

Summary

The purpose of this PR is to introduce functionality for a switch-row in settings that controls some application state for unit preferences, as part of #236. It currently does not include implementation of anything that would apply that state where applicable. Specifically, this PR:

• creates a \<ListItemSwitch> component (and related tests) that's essentially \<ListItem> but with a Switch instead of a chevron. Also, instead of the onPress prop, it has onChange and value props.

• updates the userPreferences slice and selectors (and corresponding tests) to include a useMetricUnits boolean in the Redux store with a default value of true. The toggleUnits reducer, like toggleNotification, receives the desired units setting as a boolean payload in an action, rather than just receiving state and setting useMetricUnits to the opposite of its current value.

• creates a new row in the Settings screen under About, with the English title "Metric units" (the translations were updated for all languages to reflect this, though). It is a \<ListItemSwitch>, receiving useMetricUnits's selector as its value prop and, for its onChange prop, a dispatch() of the toggleUnits action creator's returned action with the opposite of the value prop as its payload. The Settings snapshot was updated to reflect this change.

Note: Even though #236 indicates that the switch might literally display with the text "Imperial units," I assumed more users would recognize the term "Metric units" and understand that toggling it off would switch the app to that set of units used by "english citizen[s] or ex british colony citizen[s]" (i.e. the imperial system). Further, I omitted the word "use" (in the sense of "Use metric units") for simplicity in translation.

Demo

[ghost]

👇 Click on the image for a new way to code review

#### [](https://app.codesee.io/r/reviews?pr=365&src=https%3A%2F%2Fgithub.com%2FNMF-earth%2Fnmf-app) #### Legend

[PierreBresson]

Looks good :)

[ThomasLatham]

Okay thanks @PierreBresson!

[ThomasLatham]

@PierreBresson could you please take a look at d03df83? It includes the implementation of getImperialMetricValue() in the calculation util as discussed here in #236 as well as application of the useMetricUnits state in the emission-adding components.

In implementing the application of useMetricUnits in those components, I discovered a variance in the way they called their respective emission-setting functions passed in through props (e.g. setElectricityConsumption() in the electricity emission-adding component, and setQuantity() in the fashion emission-adding component). To use these two as an example (although the difference was apparent in multiple components), pre-commit the electricity component called setElectricityConsumption() directly in the component (so on re-render, essentially), while the fashion component called setQuantity() in an onSliderValueChange() function passed as its slider's onSlidingComplete prop. Both methods effectively cause the respective methods to be called when the slider changes, but the former way caused a warning to arise when accessing those components in development mode, and the latter way seemed (to me at least) to establish more intention and organization in the code, even if just a little bit. So I standardized that pattern across the components.

---

EDIT

Since the streaming component has some extra complexity for determining which values/units to display, I deviated from the patterns established in the other components by extracting some of that logic to immediately called functions in the top section of the component, rather than keeping it in its return statement. So it has some variables/functions,

 const displayCarbonValue = (() => {
    if (carbonValue > 1 && useMetricUnits) {
      return carbonValue;
    } else if (carbonValue <= 1 && useMetricUnits) {
      return carbonValue * 1000;
    } else  {
      return getImperialMetricValue(carbonValue, useMetricUnits, MeasureType.mass);
    } 
  })();

  const displayUnits = (() => {
    if (carbonValue > 1 && useMetricUnits) {
      return "kgCO2eq";
    } else if (carbonValue <= 1 && useMetricUnits) {
      return "gCO2eq";
    } else  {
      return "lbsCO2eq";
    } 
  })();

in the top section of the component, instead of implementing that logic with nested ternaries in the returned TSX section of the component. I used immediately called functions so that the associated variables could be consts, rather than declaring lets and then changing their values accordingly.

END EDIT

---

Lastly, I'm not sure how/where to go about testing these changes (and I admittedly haven't given it a whole lot of thought), but it seems to me that I should write some tests to assert that the actual presented values are as expected given either value of the useMetricUnits state. If my suspicions are correct, and tests are indeed in order (are they?), then can I do some brainstorming about how to implement them and then present my plan to you for approval?

[PierreBresson]

I've noticed some spacing issues across several files

I think you have to enable format on save on your ide/vscode settings, it should solve the issue

[PierreBresson]

I think that standardized pattern that you have applied across the components is a good thing, let's keep it. For displayCarbonValue and displayUnits maybe we could add them into calculation, as it would be handy to have them available for other components and avoid duplicate code and improve rendering. For example, I'm wondering if it would not be better to display other units because if a value is not in 1-1000 range, then people might be lost, that's why I've display gram/kg in some places. However, I'm not familiar with imperial units, so I don't know what would be the best unit to display Here are some suggestions that you can improve :

[PierreBresson]

Regarding the tests, there is correctly no test checking the entire add emission flow, so it's fine if you don't add any. I'm planning to change the slider to some input field at some point and I will add more tests then. If you want to move displayCarbonValue and displayUnits to improve UX, then I would suggest testing these two functions as they would also be more complex in order to display several kind of units.

[PierreBresson]

Here are the places still using metric, but after that we are good to merge and release 🙂

Let me know if you need some help with anything, and thanks a lot for the help ;)

[ThomasLatham]

I think that standardized pattern that you have applied across the components is a good thing, let's keep it. For displayCarbonValue and displayUnits maybe we could add them into calculation, as it would be handy to have them available for other components and avoid duplicate code and improve rendering. For example, I'm wondering if it would not be better to display other units because if a value is not in 1-1000 range, then people might be lost, that's why I've display gram/kg in some places. However, I'm not familiar with imperial units, so I don't know what would be the best unit to display

I learned something neat today in researching this: the ton used in America (and some industries in Canada) is different from the ton used in the UK and Commonwealth of Nations. Here, "ton" references the short ton, equivalent to 2,000 lb, but in the UK, "ton" references the long ton, equivalent to 2,240 lb (https://en.wikipedia.org/wiki/Ton). Would you want me to separate the unit preference into 3 choices (Metric, Imperial and US/CAN) to accommodate for this difference? Alternatively, and in my own opinion as a user of Imperial system, I don't think it's such a hindrance to keep using pounds even when the scale might permit converting to tons (although it is a pretty common practice to switch as such, at least in my own country). Either way, I'm happy to implement what's best for the app.

I'll implement a conversion to ounces for small scales. Personally, I typically think in terms of fractional pounds, even when presented with a measurement in ounces (e.g. if I see an 8 oz. steak on a menu, I'm thinking, "Oh, like half a pound.") since that's where I have the most context, but that might not be true for all subjects of the Imperial system, and it's not uncommon to see the weight of food products given in ounces.

---

EDIT

I also found out that while "lbs" is an acceptable abbreviation for the plural of "pound" for common purposes, technically the proper abbreviation is "lb". I'll go ahead and make the code reflect this change if you'd like.

EDIT 2

The 3 options I above referenced as "Metric, Imperial and US/CAN" might be better described as "Metric, Imperial and United State customary units."

[ThomasLatham]

I've noticed some spacing issues across several files

I've got format-on-save configured now in my settings.json. Not sure if I'm using the correct format standard though. I tried one, but it removed all lines between import groups, so I tried another, and it removed spacing in the code while maintaining the separation of imports.

[ThomasLatham]

Regarding the tests, there is correctly no test checking the entire add emission flow, so it's fine if you don't add any. I'm planning to change the slider to some input field at some point and I will add more tests then. If you want to move displayCarbonValue and displayUnits to improve UX, then I would suggest testing these two functions as they would also be more complex in order to display several kind of units.

Okay, I extracted those functions as getDisplayUnitsValue and getDisplayUnits, respectively, so I'll test them too. Do you have any feedback on my implementation and usage of them?

EDIT

Wrote tests for the functions I added to calculation.ts: getImperialMetricValue, getDisplayUnitsValue and getDisplayUnits

[ThomasLatham]

Here are the places still using metric, but after that we are good to merge and release 🙂 Let me know if you need some help with anything, and thanks a lot for the help ;)

Didn't quite get to these in the last push, although I certainly intend to. Thanks for the opportunity to contribute. If I'm moving too slowly with this feature, I won't have any hard feelings if you jump in to wrap it up

EDIT

Implementing units based on user preference for the country list in the Monthly Emissions screen is making me want to provide localization for the unit abbreviations. Also, I'd like to add a boolean argument to the getDisplayUnits() function to return the full name for the unit instead of the symbol (e.g. "kilograms" vs "kg"), with respect to the current locale. I might be kind of reinventing the wheel here with localization of the units. This looks helpful. But I definitely don't need that whole "wheel," just a few spokes, so to speak. Plus it doesn't look like it has support beyond symbols if you're good with me adding support for full unit names in addition to the symbols.

[ThomasLatham]

Okay, for all I wrote in the comments above, I think now I can say I've got everything in a place ready for review.

[PierreBresson]

I think that standardized pattern that you have applied across the components is a good thing, let's keep it. For displayCarbonValue and displayUnits maybe we could add them into calculation, as it would be handy to have them available for other components and avoid duplicate code and improve rendering. For example, I'm wondering if it would not be better to display other units because if a value is not in 1-1000 range, then people might be lost, that's why I've display gram/kg in some places. However, I'm not familiar with imperial units, so I don't know what would be the best unit to display

I learned something neat today in researching this: the ton used in America (and some industries in Canada) is different from the ton used in the UK and Commonwealth of Nations. Here, "ton" references the short ton, equivalent to 2,000 lb, but in the UK, "ton" references the long ton, equivalent to 2,240 lb (https://en.wikipedia.org/wiki/Ton). Would you want me to separate the unit preference into 3 choices (Metric, Imperial and US/CAN) to accommodate for this difference? Alternatively, and in my own opinion as a user of Imperial system, I don't think it's such a hindrance to keep using pounds even when the scale might permit converting to tons (although it is a pretty common practice to switch as such, at least in my own country). Either way, I'm happy to implement what's best for the app.

I'll implement a conversion to ounces for small scales. Personally, I typically think in terms of fractional pounds, even when presented with a measurement in ounces (e.g. if I see an 8 oz. steak on a menu, I'm thinking, "Oh, like half a pound.") since that's where I have the most context, but that might not be true for all subjects of the Imperial system, and it's not uncommon to see the weight of food products given in ounces.

EDIT

I also found out that while "lbs" is an acceptable abbreviation for the plural of "pound" for common purposes, technically the proper abbreviation is "lb". I'll go ahead and make the code reflect this change if you'd like.

EDIT 2

The 3 options I above referenced as "Metric, Imperial and US/CAN" might be better described as "Metric, Imperial and United State customary units."

I'm good with not display imperial tons, as I think it would introduce too much complexity, and this current implementation is good enough to be released.

[PierreBresson]

Yeah, I think too we should keep as it is now or it would be re-inventing the wheel. It would be nice to find a library like the .net for js so we could have the rendering of the units based on the phone settings, like the language, or through a list of option.

[PierreBresson]

Okay, for all I wrote in the comments above, I think now I can say I've got everything in a place ready for review.

Correct, I've played with the app and everything seems to be working/implemented in all the screens, well done 👏 Just little adjustments we can merge :) Thanks a lot!

[ThomasLatham]

Hey @PierreBresson is this by any chance ready to merge?

[PierreBresson]

@ThomasLatham It's merged! 🥳 Let me know if you want some stickers by email, I will send you some ;) I'm going to make a release soon with this feature. Thanks a lot 🙂

[ThomasLatham]

@PierreBresson Thanks so much! That would be awesome to get some stickers :D This was basically my first moderate-size contribution to an open-source project (everything before was basically one or two lines). I learned a ton and am really grateful for the opportunity!