fix: Table visualization shows total for duration, percentage and bytes field formatters#54240
Conversation
mbondyra
added
Team:Visualizations
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
mbondyra
added
the
release_note:skip
|
@elasticmachine merge upstream |
….com:mbondyra/kibana into IS-50036_show-sum-for-a-field-with-formatter * 'IS-50036_show-sum-for-a-field-with-formatter' of github.com:mbondyra/kibana: [Telemetry] Fix license page crashing on telemetry.enabled: fa… (elastic#54174) [ui/public/utils] Copy rarely used items to where they are consumed (elastic#53819) set AppArch team as an owner of the search endpoints (elastic#54131) Don't expose Elasticsearch client as Observable (elastic#53824) [SIEM] Cleanup unnecessary use of enzyme-to-json (elastic#53980)
mbondyra
changed the title
| }; | ||
|
|
||
| const isDate = checkDimensionType(dimension, 'date'); | ||
| const isNumeric = shouldShowTotal(dimension); |
There was a problem hiding this comment.
This technique is pretty brittle- it says that there is a hardcoded list of formatters that can be used by the table, instead of looking at some property of the formatter. For example, I am adding new formatters: #53972
Is there a change we could make to the formatters API? We could take a different technique and add another property to number formats that would provide the formatted and raw data, but this might already exist?
There was a problem hiding this comment.
Hi @wylieconlon thank you for your comment, good point! How about adding additional property allowsNumericalAggregations for all the types we need NumberFormat, PercentFormat and BytesFormat and DurationFormat? Please, re-check my changes and let me know what you think.
|
Yes, I found the change I'm thinking of. We already apply formatting and totals in separate ways because otherwise it's buggy: #48090 |
lizozom
left a comment
lizozom
left a comment
There was a problem hiding this comment.
I added a small comment on the implementation, however overall LGTM
|
|
||
| id = BytesFormat.id; | ||
| title = BytesFormat.title; | ||
| allowsNumericalAggregations = BytesFormat.allowsNumericalAggregations; |
There was a problem hiding this comment.
Overall looks ok, However I don't understand why the attribute is set on both statically and as an attribute. I would opt for either of the options, but not both, unless there's specific reason to do so.
For example, if you prefer the static attribute, you could still access it in agg_table like this: formatter?.constructor.allowsNumericalAggregations. Or you can just set allowsNumericalAggregations = true and then access it as usual.
wylieconlon
left a comment
wylieconlon
left a comment
There was a problem hiding this comment.
This change LGTM, although I didn't test it locally
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
6 participants
Summary
Fixes #50036
Currently, we only show a total for number formatter. We also want to display it for duration, percentage and bytes.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately