Bug in Weather Units for Broadcasted Notification #3519
Conversation
…mperial method and add comments
|
Thanks! Can you please add an entry to the |
|
if you need help to fix the lint errors in the failed test let me know |
|
Some of the problem is doing npm run install-mm doesn’t install the git commit hooks |
|
I resolve eslint errors. |
|
from the test: yes, you introduced |
|
looks like a typo:
|
|
Thanks very much for your time and help, my mistake is that I skipped running tests, because I was working on Windows system. |
| */ | ||
| convertPrecipitationToInch (value, valueUnit) { | ||
| if (valueUnit && valueUnit.toLowerCase() === "cm") return value * 0.3937007874; | ||
| else return value * 0.03937007874; |
There was a problem hiding this comment.
this else code does the same as the if part?
and why not use the convertPrecipitationUnit method?
There was a problem hiding this comment.
the code inside if converts cm to inch and the code inside else converts mm to inch.
The reason that I added convertPrecipitationToInch () method is that convertPrecipitationUnit() returns a string, and I think that maybe we need the raw value of precipitation.
There was a problem hiding this comment.
sorry for late reply and my first wrong response...
i get your point, will add a small improvement myself and approve
| */ | ||
| convertPrecipitationToInch (value, valueUnit) { | ||
| if (valueUnit && valueUnit.toLowerCase() === "cm") return value * 0.3937007874; | ||
| else return value * 0.03937007874; |
There was a problem hiding this comment.
sorry for late reply and my first wrong response...
i get your point, will add a small improvement myself and approve
This PR resolve Issue number #3419 .
I have added the method
convertWeatherObjectToImperial()which converts the units of thenotificationPayloadto imperial if needed, in order to pass the object insendNotification().