[Lens] Add suffix formatter#82852
Conversation
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
wylieconlon
left a comment
There was a problem hiding this comment.
It looks like the field-specific default formatter is not being respected in all cases. Steps to reproduce:
- Apply the code change you suggested
- Drag a numeric field into the workspace
- Hover over the tooltip
Other than this issue the behavior LGTM
|
@elasticmachine merge upstream |
…nto lens/suffix-formatter
|
@wylieconlon Thanks for the review, the nested params were not passed along the right way. I fixed it by explicitly handling the separate cases.
|
dej611
left a comment
There was a problem hiding this comment.
Not sure I proposed an improvement over the current one, but I got lost parsing all the branchings in the format_column body
| return withParams(col, { | ||
| ...col.meta.params, | ||
| params: { | ||
| ...innerParams, | ||
| ...parentFormatParams, | ||
| }, | ||
| }); | ||
| } else { | ||
| // original format is not nested, wrapping it insifr parentFormatId/parentFormatParams | ||
| return withParams(col, { | ||
| ...col.meta.params, | ||
| id: parentFormatId, | ||
| params: { | ||
| id: col.meta.params?.id, | ||
| params: innerParams, | ||
| ...parentFormatParams, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
I find this bit a bit hard to wrap my head around.
I would propose something alternative:
const isNested = isNestedFormat(col.meta.params);
const innerParams = isNested ? {id: col.meta.params?.id, params: col.meta.params?.params } : col.meta.params?.params;
const formatId = isNested ? col.meta.params.id : parentFormatId;
return withParams(col, {
...col.meta.params,
id: formatId,
params: {
...innerParams,
...parentFormatParams
}
});There was a problem hiding this comment.
@dej611 I refactored the code like this, however there is a bug in there - the first ternary condition calculating the innerParams is flipped (it should be the nested params if the original formatter was nested, not the other way around)
|
Functionality works great and code looks good to me. The only thing that doesn't work so well is the test case provided (try switching from count of records or cardinality to average - the suffix won't be applied). The right solution would be in this case to add suffix in both cases: It doesn't influence the working code though so PR approved :) |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
wylieconlon
left a comment
There was a problem hiding this comment.
Haven't tested it or seriously reviewed again, but I don't want to block it any more.
* master:
[Ingest Manager] Lift up registry/{stream,extract} functions (elastic#83239)
[Reporting] Move "common" types and constants to allow cross-plugin integration (elastic#83198)
[Lens] Add suffix formatter (elastic#82852)
[App Search] Version documentation links (elastic#83245)
Use saved object references for dashboard drilldowns (elastic#82602)
Btsymbala/registered av (elastic#81910)
[APM] Errors table for service overview (elastic#83065)
Fixes #76714
This PR adds a new "suffix" formatter which works similar to the range formatter. It's taking a nested serialized field format object and uses it to format the value itself, then adds the suffix in the back. As a param it takes a unit which can be passed to the time_scale function as well.
This PR doesn't handle the label change - I imagine this to be part of the PR piecing all the parts together (UI to enable this, setting the label, setting the outer formatter, writing the
time_scaleexpression function)Testing
To test, replace the
buildColumnfunction inx-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/metrics.tsxby this:Then configure a chart using average/min/max/median operations.
Related changes
In
x-pack/plugins/lens/public/indexpattern_datasource/format_column.ts, I had to make sure the id of the outer format is used correctly. This wasn't an issue for range because it only replaced params on an already existing outer format.