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

Missing tooltip data #2753

Merged
merged 4 commits into from
Oct 29, 2024
Merged

Missing tooltip data #2753

merged 4 commits into from
Oct 29, 2024

Conversation

wakek
Copy link
Contributor

@wakek wakek commented Sep 16, 2024

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.

Signed-off-by: Albert Kyei <akyei908@gmail.com>
Signed-off-by: Albert Kyei <akyei908@gmail.com>
@wakek
Copy link
Contributor Author

wakek commented Oct 7, 2024

Hi @texodus, could you take a look this PR to let me know if anything is amiss, thanks.

@sinistersnare
Copy link
Contributor

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.

{
  "version": "3.1.0",
  "plugin": "X/Y Line",
  "plugin_config": {},
  "columns_config": {},
  "settings": true,
  "theme": "Pro Light",
  "title": null,
  "group_by": [
    "exam_month"
  ],
  "split_by": [
    "exam_subject",
    "teacher"
  ],
  "columns": [
    "exam_month",
    "exam_score",
    null
  ],
  "filter": [],
  "sort": [],
  "expressions": {},
  "aggregates": {
    "exam_score": "avg",
    "exam_month": "any"
  }
}

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!

@texodus texodus added the bug Concrete, reproducible bugs label Oct 17, 2024
Copy link
Member

@texodus texodus left a 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

@wakek
Copy link
Contributor Author

wakek commented Oct 17, 2024

@sinistersnare Thanks for the feedback and suggestions. I'll work on it further.

wakek added 2 commits October 21, 2024 13:03
Signed-off-by: Albert Kyei <akyei908@gmail.com>
Signed-off-by: Albert Kyei <akyei908@gmail.com>
@wakek wakek requested a review from texodus October 22, 2024 06:48
Copy link
Member

@texodus texodus left a 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!

@texodus texodus merged commit fb11915 into finos:master Oct 29, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Concrete, reproducible bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants