-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Missing tooltip data #2753
Missing tooltip data #2753
Conversation
Signed-off-by: Albert Kyei <akyei908@gmail.com>
Signed-off-by: Albert Kyei <akyei908@gmail.com>
Hi @texodus, could you take a look this PR to let me know if anything is amiss, thanks. |
Thanks for submitting this! Although this succeeds for simple cases, it does not work for more complex examples. Luckily the code you wrote is on the right path, it only needs to be generalized further. Here is a config that shows that your fix is not enough. Using the example data from #2575 and the file example, providing more than 1 split overwhelms your fix. Instead of hardcoding one level of split, you need to allow for any number of splits.
To apply the config you can use the debug panel provided in the top right of the sidebar. Also, We generally require a unit test for any fix, regardless of complexity. If you could write a unit test that ensures that this functionality works, it would be amazing! Again thanks for your work in fixing this issue, I hope we can get this merged soon! |
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.
See @sinistersnare's feedback pls
@sinistersnare Thanks for the feedback and suggestions. I'll work on it further. |
Signed-off-by: Albert Kyei <akyei908@gmail.com>
Signed-off-by: Albert Kyei <akyei908@gmail.com>
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 PR! Looks good!
This PR addresses the bug reported in #2575 and #2254 where tooltip data labels were missing their corresponding values when multiple data values are configured. The issue stemmed from an invalid property used to fetch tooltip data from the
data.row
JSON, which was missing a required “prefix”. This has now been corrected.