Table: Fix units showing in footer after reductions without units #82081
Conversation
codeincarnate
requested review from
nmarrs and
baldm0mma
and removed request for
a team
|
Did notice that this messes up percentage formatting where percents are rounded. Currently considering what to do there. |
nmarrs
changed the title
There was a problem hiding this comment.
Overall LGTM - not sure on edge case you're encountering with rounded percents (do you have a dashboard you can share to replicate that?)
In testing each reducer type the units are preserved / ignored as I would expect 👍
debug-Panel Title-2024-02-08 12_20_29.json.txt
|
This is the approach we've taken before to address this (at least in XY Chart) If we move forward with these changes we may need to do a review and cleanup of handling like this (can be follow-up) |
|
Thanks for the review Nathan! Interesting that another go was taken at this. I'll follow up so that we can unify approaches. I believe we should go with the option presented here so that it can be handled in a uniform way regardless of usage. |
|
|
||
| // If the reducer preserves units then format the | ||
| // end value with the field display processor | ||
| if (reducerInfo.preservesUnits) { |
There was a problem hiding this comment.
Just wanted to leave a comment here that this would be the area to refactor and pull out into a separate function in this part so we can use in other places (i.e. XY chart, etc.)
|
Oof, did run into one issue. Panel crashes when the reducer is undefined 😢. Fixing that now. |
|
Here's what I was seeing the the Old: New: Basically we've been applying at least some rounding to these numbers. This is a change in behavior, that being said this may be better suited as an option of some kind, such as setting number of significant digits. I can see arguments for rounding and for not rounding depending on use case. Would definitely be interested to thoughts on that 🙂 |
|
Fix the issue when there's an undefined value for the footer 🥳 |
| @@ -41,6 +41,7 @@ export interface FieldReducerInfo extends RegistryItem { | |||
| // Internal details | |||
| emptyInputResult?: unknown; // typically null, but some things like 'count' & 'sum' should be zero | |||
| standard: boolean; // The most common stats can all be calculated in a single pass | |||
| preservesUnits: boolean; // Whether this reducer preserves units, certain ones don't e.g. count, distinct count, etc, | |||
There was a problem hiding this comment.
Thanks for the comment here.
| } | ||
|
|
||
| // Calculate the reduction | ||
| const format = field.display ?? getDisplayProcessor({ field, theme }); |
There was a problem hiding this comment.
Also appreciate the more verbose renaming here.
|
Re: percent issue is there a way for us to set the resulting value's unit to percent? This reminds me of some work that we've done / issues that we ran into previously with an issue with bar chart percent stacking 😬 IMO in it's current iteration it is better than before so I would be fine with us merging and creating an issue (or two) to follow up on:
|
|
@nmarrs cool, sounds good there. I'll go ahead and get this merged up 😄 |
…2081) * Add preservesUnits property to reducer registry items * Hide units when appropriate * Prettier * some code cleanup * Prevent error when no stat is selected. --------- Co-authored-by: nmarrs <nathanielmarrs@gmail.com>
…afana#82081) * Add preservesUnits property to reducer registry items * Hide units when appropriate * Prettier * some code cleanup * Prevent error when no stat is selected. --------- Co-authored-by: nmarrs <nathanielmarrs@gmail.com>
This PR fixes an issue in which table footer items will display units even in cases where the reduction will not preserve units. i.e. in cases where counts are applied of percentages are calculated.
This does so by adding a
preservesUnitsproperty to reducer registry items so that it's possible to determine if unit formatting should be applied after reduction.Fixes #81670
Special notes for your reviewer:
Please check that: