-
Notifications
You must be signed in to change notification settings - Fork 240
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
feat: add value_multiplier and extend value_factor #608
base: master
Are you sure you want to change the base?
Conversation
Is there anything missing to complete this PR? |
@maulwuff I don't think so. |
The review comments need to be addressed before we can merge this. |
There are no review comments..? |
@@ -5,7 +5,19 @@ import { | |||
} from './const'; | |||
|
|||
export default class Graph { | |||
constructor(width, height, margin, hours = 24, points = 1, aggregateFuncName = 'avg', groupBy = 'interval', smoothing = true, logarithmic = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not need to pass valueFactor
& valueMultiplier
here, they aren't used in the class.
|
||
const x = 10 ** dec; | ||
return (Math.round(state * value_factor * x) / x).toFixed(dec); | ||
return (Math.round(state * value_factor * x) / x).toFixed(dec) * value_multipler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The decimals
option will not play nicely together with this change as we first round the value and then multiply with the multiplier value. Should apply the multiplier first and then do the rounding.
@@ -666,12 +667,14 @@ class MiniGraphCard extends LitElement { | |||
} | |||
const dec = this.config.decimals; | |||
const value_factor = 10 ** this.config.value_factor; | |||
const value_multipler = this.config.value_multipler || 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to add this as an option on the entity config level we need to check the entity specific config here as well.
@OmgImAlexis come on, you can fix that ;-) |
Please also add documentation for the option in the readme 👍🏼 |
Would love to see this in mini-graph-card, currently this prohibits me from making a total power draw card. My total consumption is reported in kW, while individual devices report in W. |
In the meantime you could look at creating a template_sensor and handle the conversion there. |
@OmgImAlexis Do you need some help addressing these (minor) comments & getting this merged? |
Please check this: #591 (comment) |
This adds value_multiplier which similarly to the value_factor only it multiplies the value directly. By default it's set to
1
.I've also added the
value_factor
to the entity itself to allow different factors for each entity. Personally I like this as it allows cards to show more than a single type of data. For example I want to show data usage and power usage on one, the issue is one is in bytes(I want MB) and one is watts(I want KW). I need one to be divided by1024
and the other1000
.I haven't tested this yet so please let me know if there's anything I need to fix.
Closes #607 and #591