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
Units: Add scalable unit option #79411
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Develer 👋🏾 , great job tackling this PR, I gave it a test locally and it works as expected 🥳 .
From my point of view is almost ready, would be great if users could a short description in the UI about this option.
As well, we are missing a few things:
- Documentation regarding the new option, you can do that here: https://github.com/grafana/grafana/blob/main/docs/sources/panels-visualizations/configure-standard-options/index.md?plain=1#L57
- Some unit test :) (we have
symbolFormatters.test.ts
andvalueFormats.test.ts
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some docs and a few tests :) overall LGTM :)
I really like how you did not change the default behavior and instead made this opt-in functionality - this helps us to prevent messing up users' dashboards 💯
Screen.Recording.2024-01-01.at.5.07.47.PM.mov
{ name: 'metric ton (t)', id: 'masst', fn: toFixedUnit('t') }, | ||
], | ||
}, | ||
{ | ||
name: 'Length', | ||
formats: [ | ||
{ name: 'millimeter (mm)', id: 'lengthmm', fn: SIPrefix('m', -1) }, | ||
{ name: 'millimeter (mm)', id: 'lengthmm', fn: SIPrefix('m', -1, scalable) }, | ||
{ name: 'inch (in)', id: 'lengthin', fn: toFixedUnit('in') }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inches / feet (and other non worldwide standard measures (fluid ounces etc) can also be considered scalable 🤔 (i.e. 12 inches in 1 ft, 5280ft in 1 mile) - though it seems like our system currently only supports metric scaling 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, indeed, right now it's only system international related units 🙂 We have to implement a separate scaling function for inches and feet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update to the docs. Two thoughts:
- What's written there makes sense, but I don't understand the underlying logic of what triggers the scaling of units. Should we explain that logic a bit?
- Since we're making this default behaviour, and will change the appearance of people's visualizations, I think it warrants a What's new entry.
Before adding the scaling option, units have scaled by default, so there are no significant changes for the user. Technically, we only added the option to turn off scaling 😁 |
Ah okay, I understand. Then it's fine as is. |
What is this feature?
This PR provides scale option for unit standard edit option
Which issue(s) does this PR fix?:
Fixes #50975
Special notes for your reviewer:
Please check that: