Show the sum of the power usage in power track's tooltips#4172
Conversation
| .label = Power | ||
|
|
||
| TrackPower--tooltip-power-used-in-range-watthour = { $value } Wh | ||
| .label = Power used in range |
There was a problem hiding this comment.
What's range in this context?
| .label = Power used in range | ||
|
|
||
| TrackPower--tooltip-power-used-in-preview-watthour = { $value } Wh | ||
| .label = Power used in temporary selection |
There was a problem hiding this comment.
I don't think temporary is the right choice here, or at least I'm confused on the meaning. Does it refers to the fact that this is the power consumption for a specific time range?
There was a problem hiding this comment.
In the UI, there's a distinction between the "committed" range, and the preview selection. From the preview selection we can either click "+" to make it the "committed" range (and pass it full screen), or click elsewhere to unselect it. That's what I call the "temporary" selection. But I'm not especially happy about that either.
Our internal term is "preview selection" but I'm not sure this is understandable either.
There was a problem hiding this comment.
I think using the word "Energy" instead of "Power" for the amount of energy used over a range would be more correct.
Ideas of alternative wordings:
"Energy used in the visible range" & "Energy used in the selected range"
And I'm not sure about the word "visible", I also considered "full", "entire", "current". None of them feels perfect. "committed range" is technically correct but too jargonish.
There was a problem hiding this comment.
I used Energy used in the visible range and Energy used in the current selection. Not especially obvious but not bad either. I guess the meaning is well discoverable with testing.
Codecov Report
@@ Coverage Diff @@
## main #4172 +/- ##
==========================================
+ Coverage 88.43% 88.44% +0.01%
==========================================
Files 280 281 +1
Lines 24564 24587 +23
Branches 6557 6559 +2
==========================================
+ Hits 21722 21747 +25
+ Misses 2640 2638 -2
Partials 202 202
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| return sum * 1e-12; | ||
| } | ||
|
|
||
| _computePowerSumForCommittedRange = memoize( |
There was a problem hiding this comment.
I decided to memoize here rather than in redux, to make the component more self contained. I thought this wouldn't be used elsewhere, at least not yet. Maybe we may want to display it elsewhere in the future, but we'd probably need to compute the sum of all tracks too... A problem for another day...
| # preview selection uses the watt-hour unit. Variables: | ||
| # $value (String) - the power value at this location | ||
| TrackPower--tooltip-power-used-in-preview-watthour = { $value } Wh | ||
| .label = Power used in temporary selection |
There was a problem hiding this comment.
What is the difference between "range" and "temporary selection"? Maybe change "temporary" to "current".
There was a problem hiding this comment.
Here is what I explain above:
In the UI, there's a distinction between the "committed" range, and the preview selection. From the preview selection we can either click "+" to make it the "committed" range (and pass it full screen), or click elsewhere to unselect it. That's what I call the "temporary" selection. But I'm not especially happy about that either.
I used current instead of temporary like you suggested :-)
|
Hey @canova, this is now ready for review! (again :-) ) |
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Looks pretty good to me, thanks! I have a nit about the position of the items but let me know what you think.
| return sum * 1e-12; | ||
| } | ||
|
|
||
| _computePowerSumForCommittedRange = memoize( |
There was a problem hiding this comment.
Good idea to memoize these values!
|
|
||
| return ( | ||
| <Localized id={l10nId} vars={{ value }} attrs={{ label: true }}> | ||
| <TooltipDetail label="">{value}</TooltipDetail> |
There was a problem hiding this comment.
I guess this label is here to make the flow happy, right?
| {previewSelection.hasSelection | ||
| ? this._formatPowerValue( | ||
| this._computePowerSumForPreviewRange(previewSelection), | ||
| 'TrackPower--tooltip-energy-used-in-preview-watthour', | ||
| 'TrackPower--tooltip-energy-used-in-preview-milliwatthour' | ||
| ) | ||
| : null} |
There was a problem hiding this comment.
I feel like this could be above the committed range in the tooltip (as in in the second place instead of third).
The reason behind it to me is that the value in the committed range doesn't change during the selection. Only 1st and 3rd values change, I think grouping the frequently changing numbers together would make more sense in my head. What do you think?
There was a problem hiding this comment.
I didn't really know the right order and changed it frequently while developing, so I'm glad you have an opinion :-) I'll change it.
…ack power's tooltip
…r tooltip component
5 participants
deploy preview
I won't be able to change it more before my holidays but I believe it's fairly ready now. Please change the labels if needed and then merge when approved :-)
Please look at the individual commits. The first commits are "technical" in that they're extracting the tooltip rendering into its own component and file, so that we won't rerender the graph for each new preview selection.
Fixes #4154