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

Refresh the status checks before the status modal is opened #811

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

shankari
Copy link
Contributor

@shankari shankari commented Feb 9, 2022

  • fix formatting for the mph values

Before this change, the status was only computed when the controller was
initialized. This is fine for all permission related changes, since downgrading
permissions causes the app to restart and the controller to get re-initialized.
However, changing location settings does not cause the app to restart, and
presumably other status checks might not as well.

So if we initialize the controller (maybe with green values) and then fail the
status again, then it stays green.

The principled fix for this is probably a service, which will also allow us to
read the status and change the color and icon accordingly.
e-mission/e-mission-docs#680 (comment)

But for now, a decoupled approach, with event-driven message passing is be a
better option. We can revisit when we refactor the app status code into
individual components.

If we choose to keep an event-driven architecture, we can also use events to
get the overall status back to the parent component.
Without this fix, the mph values had lots of decimal points.
Note that we might not actually need to `parseFloat`, multiplying with numeric
strings seems to work fine at least in chrome.

```
ic.getKmph(metersPerSecond)
"10.80"
KM_TO_MILES  * ic.getKmph(metersPerSecond)
6.7108068
KM_TO_MILES  * 10.80
6.710806

KM_TO_MILES * "3.1"
1.9262501
```

But I presumably made the change for a reason, so let's be consistent throughout and change later if needed
e-mission@98af95d
@shankari shankari merged commit 521c458 into e-mission:master Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant