Skip to content
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

[Lens] Add suffix formatter #128246

Merged
merged 5 commits into from
Mar 28, 2022
Merged

[Lens] Add suffix formatter #128246

merged 5 commits into from
Mar 28, 2022

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Mar 22, 2022

Fixes #88402

This PR adds a simple way to add a suffix to a formatter:
Screenshot 2022-03-22 at 12 14 18

  • "Suffix" input is shown for all non-default formatters
  • Can also be applied if time scaling is active (will nest to suffixes, one for the time unit, one for the user supplied string)
  • Extended the existing suffix formatter with an option to provide a string

Open question: It's a little weird to mix it with percentage or bytes formatter - should it only be shown for the "number" formatter?

@flash1293 flash1293 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens backport:skip This commit does not require backporting v8.2.0 labels Mar 22, 2022
@flash1293 flash1293 marked this pull request as ready for review March 22, 2022 16:25
@flash1293 flash1293 requested a review from a team as a code owner March 22, 2022 16:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@drewdaemon
Copy link
Contributor

Open question: It's a little weird to mix it with percentage or bytes formatter - should it only be shown for the "number" formatter?

I was trying to think of a use-case for percent/bytes. This one is sort-of viable(?)

Screen Shot 2022-03-23 at 1 55 33 PM

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and it works great. Only thing I didn't test is the following bullet (because I don't know how 😅 )

Can also be applied if time scaling is active (will nest to suffixes, one for the time unit, one for the user supplied string)

@@ -19,17 +19,18 @@ export interface BaseIndexPatternColumn extends Operation {
timeShift?: string;
}

export interface FormatParams {
export interface LensFormatParams {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly that this interface defines configuration to format values for display to the user?

If so, we could consider calling it DisplayFormatConfig or something. I'm not sure what we gain by prepending Lens since we're already in the lens plugin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ValueFormatConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it!

@mbondyra
Copy link
Contributor

It's a little weird to mix it with percentage or bytes formatter - should it only be shown for the "number" formatter?

Yeah, it feels weird for me. But I also find weird that we put formatter in between the number and time scaling unit.. I don't have a good solution with a suffix. I think it still can be useful for the user, and maybe in the future we'll solve this issue with template instead.

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it and it works as expected in Chrome. However, I am just wondering from the product side how are we going to expand on this issue - are we planning on adding prefix too, or template formatting similar to TSVB? Do we accept the limitations that come with this solution? (and the weird behavior for time scaling too?)
If at some point we would be planning on replacing it with template, maybe it makes sense to prepare saved object shape for it by replacing the part

"format": {
    "id": "number",
    "params": {
        "decimals": 0,
        "suffix": "$"
    }
}

to:

"format": {
    "id": "number",
    "params": {
        "decimals": 0,
        "template": "{{value}}$"
    }
}

and parse the template to get the suffix? Just a thought though, maybe I am overthinking it.
If the product is ok with this solution, I am too :)

@flash1293
Copy link
Contributor Author

We are definitely in need of a larger scope refactoring of how formatting works in general today in Kibana overall. There have been some early discussions, but it's not planned for the medium term - this PR (just allowing the suffix) is meant as a stopgap measure to solve a certain common use case now until we can revisit the larger issue. We can wait until @ghudgins gets back before merging to make sure this is the direction we want to take now.

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.1MB 1.1MB +785.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
lens 45.4KB 45.5KB +34.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ghudgins
Copy link
Contributor

++ similar to everyone else I agree that it's a bit weird but I do think it's better to be predictable--I would hate for a user who is using % to not be able to find this setting. I think it's good to go as is

@flash1293 flash1293 merged commit a0518b6 into elastic:main Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Enable custom prefix and suffix for Y axis range values
7 participants