-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Stat: Support no value in spark line #78986
Conversation
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
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.
Hi @FOWind! Thank you for your contribution!
On closer review it seems like the original issue seems to have a viable fix via the stat value options calculation (choosing Last
vs Last*
)
Given this fix above, I am not sure if we need these code changes / what exactly these code changes are doing. Maybe I am missing something though 😅 - if I am please let me know :)
#52789 Stat panel no no value-1704906273213.json.txt
Screen.Recording.2024-01-10.at.9.05.06.AM.mov
As shown in the video, the sparkline in the stat panel doesn't draw the no value currently. With this PR's patched it will draw the null value if setted as below. |
@FOWind thank you for the response! I didn't notice the difference in the spark line on the first review 😅 |
the original issue this PR says it fixes mentions it relating to thresholds, but this PR might only be addressing it for sparkline portion but not the Stat panel background color, which is also threshold based. i think we need to address this more holistically and not just sparkline sub-component. |
@leeoniya I would agree we need to address more scenarios, but |
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.
This looks fine to me overall, but may need to revise based on the conversation above 😄
my main question is whether this needs to be applied once (above the Sparkline component), and this data shared for stat background and sparkline, instead of being done only in sparkline and then doing it a second time for stat background threshold. feels like it should be moved up, and done in one place to accommodate both scenarios. |
My vote here is that we move forward and merge this PR and address this refactor in a follow-up issue. Thank you for your contribution @FOWind! |
What is this feature?
[Add a brief description of what the feature or update does.]
Why do we need this feature?
[Add a description of the problem the feature is trying to solve.]
#52789
Who is this feature for?
[Add information on what kind of user the feature is for.]
Which issue(s) does this PR fix?:
Fixes #52789
Special notes for your reviewer:
Please check that: