fix: same-looking tooltips on charts #10548

dangreen · 2022-08-02T20:30:23Z

~~Now, if tooltip callback returns undefined, then will be used default callback.~~
~~PolarArea and Doughnut charts with multiple series shows tooltips same as other charts.~~

dangreen · 2022-08-02T20:40:48Z

@kurkle Hi. Scatter and Bubble charts don't use data.labels, so should their tooltips also look like on rest multiseries charts?

LeeLenaleee · 2022-08-02T21:52:15Z

I think its better to always show the title with the label of the dataset, not only if there is more than 1 dataset, will give a more consistent and clear look. Same goes for scatter you could use the dataset label in that case

kurkle

Looks good.

I'm on the same page with @LeeLenaleee that it would actually be better to just remove the overrides alltogether from these controllers. So it would always look the same unless customized.

@etimberg what do you think?

src/plugins/plugin.tooltip.js

kurkle

I like the fallback, I'd vote to keep it. Though it should be documented.
Could also split it into a separate PR.

kurkle · 2022-08-05T05:21:12Z

src/controllers/controller.bubble.js

    const {xScale, yScale} = meta;
    const parsed = this.getParsed(index);
    const x = xScale.getLabelForValue(parsed.x);
    const y = yScale.getLabelForValue(parsed.y);
    const r = parsed._custom;

    return {
-      label: meta.label,
+      label: labels[index] || '',


Could be that I'm missing something, but I'm not sure that is the correct label? meta.label is the dataset label while labels[index] would equal x or y in some cases and be empty in others.

meta.label

labels[index]

This feels like pretty buggy behaviour.
Maby its better to not disable all the default overrides but make them as simillar as possible.
Like the doughnut/pie tooltip is now a lot better since it tells which dataset its about but with the bubble chart atm you see a title that is not in the graph so it feels verry weird, same goes for scatter

LeeLenaleee · 2022-08-05T08:48:18Z

Changes also need to be reflected in the migration guide: https://github.com/chartjs/Chart.js/blob/master/docs/migration/v4-migration.md

etimberg

I'm onboard with removing the overrides.

LeeLenaleee · 2022-08-05T12:40:06Z

On an addition on the reaction I left on @kurkle comment, maby you can delete the labels array from the bubble and scatter samples so they dont show a wrong tooltip title.

dangreen · 2022-08-07T12:46:54Z

@LeeLenaleee done

dangreen · 2022-08-15T08:29:53Z

@kurkle @LeeLenaleee @etimberg Hi guys. Review please 🙏

LeeLenaleee

Some doc changes

LeeLenaleee · 2022-08-15T19:10:39Z

docs/migration/v4-migration.md

@@ -4,6 +4,10 @@ Chart.js 4.0 introduces a number of breaking changes. We tried keeping the amoun

 ## End user migration

+### Charts
+
+* Charts don't override default tooltip callbacks, so all chart types have the same-looking tooltips.


Suggested change

* Charts don't override default tooltip callbacks, so all chart types have the same-looking tooltips.

* Charts don't override the default tooltip callbacks, so all chart types have the same-looking tooltips.

LeeLenaleee · 2022-08-15T19:14:10Z

docs/samples/other-charts/bubble.md

      });
      chart.update();
    }
  },
  {
    name: 'Add Dataset',
    handler(chart) {
-      const data = chart.data;


keep this data, so its consistent with all the other sampels

'data' is already declared in the upper scope on line 5 column 7 . eslint(no-shadow)

@LeeLenaleee

They are both block scoped so it shouldn't matter and they are for the user also in different tabs.

CI way of running lint never fails on it but you run it in another way, idk why it fails but its weird.

Might be worth to see if we can not let the rule count for the docs then or disable it since how I see it, it does not seem to work correctly.

If it does not work it should be renamed in all the samples but my preference goes to having it like it was

LeeLenaleee · 2022-08-15T19:14:59Z

docs/samples/other-charts/bubble.md

@@ -30,11 +52,11 @@ const actions = [
  {
    name: 'Add Data',
    handler(chart) {
-      const data = chart.data;


See above comment

LeeLenaleee · 2022-08-15T19:16:25Z

docs/samples/other-charts/scatter-multi-axis.md

      });
      chart.update();
    }
  },
  {
    name: 'Add Dataset',
    handler(chart) {
-      const data = chart.data;


Same as above

LeeLenaleee · 2022-08-15T19:16:57Z

docs/samples/other-charts/scatter.md

      });
      chart.update();
    }
  },
  {
    name: 'Add Dataset',
    handler(chart) {
-      const data = chart.data;


etimberg

LGTM

I believe @kurkle is on vacation so his reviews might be delayed

kurkle · 2022-08-18T05:58:25Z

LGTM

I believe @kurkle is on vacation so his reviews might be delayed

That was true, but I'm back!

Sergiobop · 2022-12-19T12:20:17Z

What if we want to customize the tooltip on the doughnut charts (for example) adding a % calculation but not on every other chart type? How can we achieve that now? @dangreen

LeeLenaleee · 2022-12-19T12:23:31Z

You can still override the callbacks by configuring it in the options for those charts or in the defaults for that controller

Sergiobop · 2022-12-19T12:29:38Z

Thanks @LeeLenaleee , maybe we should add this info to the migration guide from 3.x

LeeLenaleee · 2022-12-19T15:14:49Z

But this did not change between v3 and v4

Sergiobop · 2022-12-19T15:20:29Z

@LeeLenaleee It's related to this line in the migration guide... right?

https://www.chartjs.org/docs/latest/migration/v4-migration.html

Charts don't override the default tooltip callbacks, so all chart types have the same-looking tooltips.

LeeLenaleee · 2022-12-19T15:25:28Z

No, that line is about the overrides we did in the code, not about what the user specified.

So in our controllers we overrode those functions but as user you could always override them again and you still can

shanecranor · 2022-12-29T15:29:52Z

Could this be related? I'm having issues with labels on bubble charts
#11023

dangreen marked this pull request as ready for review August 2, 2022 20:37

LeeLenaleee added type: enhancement breaking change labels Aug 2, 2022

LeeLenaleee added this to the Version 4.0 milestone Aug 2, 2022

kurkle reviewed Aug 3, 2022

View reviewed changes

src/plugins/plugin.tooltip.js Outdated Show resolved Hide resolved

dangreen changed the title ~~fix: same-looking tooltips on multiseries charts~~ fix: same-looking tooltips on charts Aug 4, 2022

dangreen requested a review from kurkle August 4, 2022 22:05

kurkle reviewed Aug 5, 2022

View reviewed changes

etimberg previously approved these changes Aug 5, 2022

View reviewed changes

dangreen dismissed etimberg’s stale review via 63ccc22 August 5, 2022 17:25

dangreen added 5 commits August 7, 2022 16:54

fix: same-looking tooltips on multiseries charts

c075b1e

fix: same-looknig tooltips on all chart types

fa67585

chore: restore tooltip plugin

71eb7e8

docs: additions to migration guide

bb24729

docs: remove labels from scatter and bubble examples

9c599e8

dangreen requested review from kurkle, LeeLenaleee and etimberg August 8, 2022 09:25

LeeLenaleee reviewed Aug 15, 2022

View reviewed changes

docs: review fix

740a818

etimberg approved these changes Aug 17, 2022

View reviewed changes

kurkle approved these changes Aug 18, 2022

View reviewed changes

etimberg merged commit e7372ad into chartjs:master Aug 18, 2022

dangreen deleted the fix-tooltip-callbacks branch August 18, 2022 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: same-looking tooltips on charts #10548

fix: same-looking tooltips on charts #10548

dangreen commented Aug 2, 2022 •

edited

Loading

dangreen commented Aug 2, 2022

LeeLenaleee commented Aug 2, 2022

kurkle left a comment

kurkle left a comment

kurkle Aug 5, 2022

dangreen Aug 5, 2022

LeeLenaleee Aug 5, 2022 •

edited

Loading

LeeLenaleee commented Aug 5, 2022

etimberg left a comment

LeeLenaleee commented Aug 5, 2022

dangreen commented Aug 7, 2022

dangreen commented Aug 15, 2022

LeeLenaleee left a comment

LeeLenaleee Aug 15, 2022

LeeLenaleee Aug 15, 2022

dangreen Aug 15, 2022 •

edited

Loading

LeeLenaleee Aug 16, 2022

LeeLenaleee Aug 15, 2022

LeeLenaleee Aug 15, 2022

LeeLenaleee Aug 15, 2022

etimberg left a comment

kurkle commented Aug 18, 2022

Sergiobop commented Dec 19, 2022

LeeLenaleee commented Dec 19, 2022

Sergiobop commented Dec 19, 2022

LeeLenaleee commented Dec 19, 2022

Sergiobop commented Dec 19, 2022

LeeLenaleee commented Dec 19, 2022

shanecranor commented Dec 29, 2022

	* Charts don't override default tooltip callbacks, so all chart types have the same-looking tooltips.
	* Charts don't override the default tooltip callbacks, so all chart types have the same-looking tooltips.

fix: same-looking tooltips on charts #10548

fix: same-looking tooltips on charts #10548

Conversation

dangreen commented Aug 2, 2022 • edited Loading

dangreen commented Aug 2, 2022

LeeLenaleee commented Aug 2, 2022

kurkle left a comment

Choose a reason for hiding this comment

kurkle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeeLenaleee Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

LeeLenaleee commented Aug 5, 2022

etimberg left a comment

Choose a reason for hiding this comment

LeeLenaleee commented Aug 5, 2022

dangreen commented Aug 7, 2022

dangreen commented Aug 15, 2022

LeeLenaleee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangreen Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etimberg left a comment

Choose a reason for hiding this comment

kurkle commented Aug 18, 2022

Sergiobop commented Dec 19, 2022

LeeLenaleee commented Dec 19, 2022

Sergiobop commented Dec 19, 2022

LeeLenaleee commented Dec 19, 2022

Sergiobop commented Dec 19, 2022

LeeLenaleee commented Dec 19, 2022

shanecranor commented Dec 29, 2022

dangreen commented Aug 2, 2022 •

edited

Loading

LeeLenaleee Aug 5, 2022 •

edited

Loading

dangreen Aug 15, 2022 •

edited

Loading