Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Imperial system #236

Closed
1 task
PierreBresson opened this issue Oct 30, 2020 · 15 comments
Closed
1 task

Imperial system #236

PierreBresson opened this issue Oct 30, 2020 · 15 comments
Assignees
Labels
🆕 Feature New feature - approved by the team 🍦 Nice to have Cool, but not urgent

Comments

@PierreBresson
Copy link
Member

PierreBresson commented Oct 30, 2020

Summary

As an english citizen or ex british colony citizen, I want to be able to use the app in the imperial system instead of the international system of units (pounds instead of kilos, miles instead of kilometers...)

Proposed feature

Imperial units everywhere in the app + add in the settings a row with instead of a chevron, a switch, that will enable/disable that feature.

To Do List

Checklist, nice to have, but not mandatory

  • propose a way to switch from metric to imperial in the settings, it can be only the initial mechanism and the task could be once we agree on the way to go, divided in small chunks
@PierreBresson PierreBresson added 🆕 Feature New feature - approved by the team 🍦 Nice to have Cool, but not urgent hacktoberfest https://hacktoberfest.digitalocean.com/ and removed hacktoberfest https://hacktoberfest.digitalocean.com/ labels Oct 30, 2020
@PierreBresson PierreBresson added the hacktoberfest https://hacktoberfest.digitalocean.com/ label Sep 24, 2021
@PierreBresson PierreBresson removed the hacktoberfest https://hacktoberfest.digitalocean.com/ label Nov 1, 2021
@ThomasLatham
Copy link
Contributor

Hi @PierreBresson, I'd like to give this a shot. Can you please assign this to me?

@PierreBresson
Copy link
Member Author

Hi @ThomasLatham, thanks having a look, let me know if you have any questions ;)

@ThomasLatham
Copy link
Contributor

Okay, thanks!

@ThomasLatham
Copy link
Contributor

ThomasLatham commented Dec 24, 2022

So far I have:

  • created a component (<ListItemSwitch>) that's essentially <ListItem> but with a Switch instead of a chevron. Also, instead of the onPress prop, it has onChange and value props. I haven't yet written tests or stories for this component yet.

  • updated 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.

  • created 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. I have not updated the Settings snapshot to reflect this.

All of this is to have a switch-row in settings that controls some application state for unit preferences. I haven't implemented anything that would apply that state where applicable because I want to make sure that what I have is okay first (and write tests for everything if so). Is there a way that this code can be reviewed without submitting a PR?

@PierreBresson
Copy link
Member Author

Sounds like a good plan. GitHub doesn't support draft pr I think, so you can just submit the pr as it is, I will have a look and help if needed

@ThomasLatham
Copy link
Contributor

ThomasLatham commented Jan 1, 2023

Now that the settings switch and Redux stuff is set up, I'm going to start on the task of having units change throughout the app according to the useMetricUnits state. My plan right now is to keep that change as close to the user as possible, so to speak, so that only the display changes but behind the scenes everything is still in metric regardless of the user's unit preference (if that makes any sense). For example,

<Text.H2 darkGray>
<FormattedNumber
value={sliderValue * electricity[electricityCountry]}
maximumFractionDigits={2}
/>{" "}
<Text.Primary>kgCO2eq</Text.Primary>
</Text.H2>

could become something like

<Text.H2 darkGray>
<FormattedNumber
value={ sliderValue * electricity[electricityCountry] * (useMetricUnits ? 1 : 2.205)}
maximumFractionDigits={2}
/>{" "}
<Text.Primary>{useMetricUnits ? "kgCO2eq" : "lbsCO2eq"}</Text.Primary>
</Text.H2>

I don't know if there's a software development term for this kind of approach. This would also mean new features involving units would have to follow the same pattern. I'm definitely open to something more elegant and DRY though, or just better in some aspect that isn't immediately apparent to me. @PierreBresson what are your thoughts on this?

@PierreBresson
Copy link
Member Author

@ThomasLatham I think the general idea is good. It could be slightly improved by using some kind of converter function, so there would be no need for copying and pasting useMetricUnits ? 1 : 2.205 in several part of the apps. So, something like this :

<Text.H2 darkGray>
  <FormattedNumber
    value={getImperialMetricValue(sliderValue * electricity[electricityCountry],useMetricUnits)}
    maximumFractionDigits={2}
  />{" "}
  <Text.Primary>{useMetricUnits ? "kgCO2eq" : "lbsCO2eq"}</Text.Primary>
</Text.H2>

If useMetricUnits is true, then getImperialMetricValue returns sliderValue * electricity[electricityCountry]
If useMetricUnits is false, then getImperialMetricValue returns sliderValue * electricity[electricityCountry] * 2.205

Feel free to use a 3rd party library for the conversion, like this one or another. There is probably a better way of doing this, but it will be okay for now

@ThomasLatham
Copy link
Contributor

Oh okay cool! Do you think getImperialMetricValue should take in a string indicating the physical quantity for which units are being converted (length and mass are the only two I've come across)? Or perhaps separate functions for different physical quantities? Also I'm guessing app/utils/calculation.ts would be a good places for this/these function/s?

@PierreBresson
Copy link
Member Author

PierreBresson commented Jan 2, 2023

Correct, I forget a third parameter, which could be an enum, something like
MeasureType { mass = 'mass', length = 'length' }
and then
getImperialMetricValue(sliderValue * electricity[electricityCountry],useMetricUnits,ImperialMetricType.mass)
Yeap, calculation.ts is a good place for MeasureType & getImperialMetricValue

@ThomasLatham
Copy link
Contributor

Okay neat. Thanks for the help!

@ThomasLatham
Copy link
Contributor

Hi @PierreBresson, I've been tinkering away at this when I find the time, and I'm ready to commit changes for implementing preferred units during emission adding, but for some reason 10 unit tests are failing in app/screens/Budget/ducks/tests/BudgetScreen.selectors.test.ts. All the tests therein related to monthly carbon values are failing, with the received value being twice the expected (except in the case of electricity carbon value, where the received value is 3 and the expected is 2). I've manually traced the function calls, but I'm just not seeing where such doubling is happening down the received path -- or halving down the expected path. I'm also unsure how to use a debugger for Jest tests in a React Native project that uses Expo, but I'm looking into it. I stashed my changes such that I'm back to the state the app was in just before I submitted the draft PR for the units toggle, but still the same 10 tests are failing. I probably made some rookie mistake somewhere, but I haven't a clue where. Could you please help me out?

@PierreBresson
Copy link
Member Author

Hey,
Correct, the CI is currently not working for some reason and not able to catch errors on main branch. I've opened an issue and will have a look at it. In the meantime, feel free to push your changes, I will push a fix soon hopefully, but it's not anything wrong on your side.

@PierreBresson
Copy link
Member Author

@ThomasLatham I've fixed the tests on main branch, you can merge main it into your branch, it should work.
fyi : it was a january bug, since some emissions in tests are only "active" at the beginning of the year 😅

@ThomasLatham
Copy link
Contributor

@PierreBresson Thanks for looking into and fixing this so fast. I had a feeling it might have something to do with the Y2k+23 switch haha. Sorry I haven't been doing much with this; getting back making some progress on this feature today!

@PierreBresson
Copy link
Member Author

#365

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 Feature New feature - approved by the team 🍦 Nice to have Cool, but not urgent
Projects
None yet
Development

No branches or pull requests

2 participants