✨ Permit users to choose Celsius or Fahrenheit. #1182
Conversation
✅ Deploy Preview for dashy-dev ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Changes preview: Legend: |
| @@ -165,6 +165,11 @@ export const getValueFromCss = (colorVar) => { | |||
| return cssProps.getPropertyValue(`--${colorVar}`).trim(); | |||
| }; | |||
|
|
|||
| /* Given a temperature in Celsius, returns value in Fahrenheit */ | |||
| export const celsiusToFahrenheit = (celsius) => { | |||
| return Math.round((celsius * 1.8) + 32); | |||
There was a problem hiding this comment.
Why not :
| return Math.round((celsius * 1.8) + 32); | |
| return Math.round((celsius * 9 / 5) + 32); |
To keep style consistent with the below fahrenheitToCelsius ?
There was a problem hiding this comment.
I have no reason to prefer one form or the other.
Would you like me to make the suggested change, or are you asking merely out of curiosity?
If the latter, it is because I asked Google for the formula for the conversion, even though the reverse formula was staring me in the face and I might have worked it out myself.
If the former, I have no objections. Let me know if I should take action based on your suggestion.
There was a problem hiding this comment.
I was suggesting it for the style consistency. It's your PR, and your code is fine... My suggestion may be viewed as nitpicking ;-)
| // Justification: This is a recoverable error that developers | ||
| // should nevertheless be warned about. | ||
| // eslint-disable-next-line | ||
| console.warn('Unrecognized unit', sensor.unit); |
There was a problem hiding this comment.
There is also a this.error('msg') (defined in the WidgetMixin.js) - which will display this to the user, as well as log to console - here's an example.
But I'm happy either way :)
Thank you for contributing to Dashy! So that your PR can be handled effectively, please populate the following fields (delete sections that are not applicable)
Category:
Feature
Overview
If merged, this PR will introduce a new option for the
GlCpuTempwidget, permitting users to express a preference for either Celsius or Fahrenheit values.The underlying Glances sensor API can already be configured to report temperatures using either scale, using the
--fahrenheitcommand line argument.This PR utilizes the unit reported by the Glances API to determine whether any conversion is necessary.
Prior to this PR, the default behavior is to always display values in Celsius, whether Glances has been configured to report in Fahrenheit or not. This default behavior has been retained, as the new option is understood to have a default value of
C.Issue Number (if applicable) #00
None.
New Vars (if applicable)
A new widget option named
unitshas been introduced to theGlCpuTempwidget. Documentation for this option has been added towidgets.md.Screenshot (if applicable)
Code Quality Checklist (Please complete)
Note: There is currently a lint failure in the
src/components/Widgets/RssFeed.vuefile. This appears to be unrelated to my changes and I presently understand this to be out of scope. Please let me know if I should correct this anyway.I am unclear on whether this is a big enough "feature" to warrant a version bump. Please advise.