-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Axis: Add separate show axis option #74117
Conversation
@@ -38,6 +38,7 @@ export const defaultGraphConfig: GraphFieldConfig = { | |||
}, | |||
axisGridShow: true, | |||
axisCenteredZero: false, | |||
axisShow: false, |
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.
Should this be true by default?
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.
As I see it's disabled in TimeSeries by default. So probably we have to leave it like that for now.
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.
Great job! 🎉 This works really well :) I've included a short before / after video in the PR description showing off the fix as well as included the Panel debug snapshot dashboard
Just had one minor nit with let
vs const
and variable naming 😬
color: customConfig.axisColorMode === AxisColorMode.Series ? axisColor : undefined, | ||
}; | ||
} | ||
let axisColorOpts = { |
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.
I wonder if this naming still makes sense as this is actually more than just axis color options 🤔
maybe it would make more sense to rename this as const axisDisplayOptions
or something?
let axisColorOpts = { | |
const axisDisplayOptions = { |
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.
At first, I thought that the tweakAxis
method somehow changes axisColorOpts
and decided not to change it to const, but now I see that it accepts a newly created object + deconstructs axisColorOpts
, so we're safe 🙂 I don't know how I missed that 🤦🏻♂️, thanks for pointing it out.
And yes, I totally agree with the new name 👍🏻
…a into 72074-display-axis-option
Co-authored-by: nmarrs <[email protected]>
What is this feature?
This PR will add "Show axis" independent FieldConfig option.
Before
before.mov
After
after.mov
Which issue(s) does this PR fix?:
Fixes #72074
Special notes for your reviewer:
Please check that:
Panel debug snapshot dashboard