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

Improvements to tooltip positioners #9944

Merged
merged 6 commits into from
Dec 6, 2021

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented Dec 3, 2021

See #9930 for background. Specific changes:

  • Allow the positioner's return object to optionally include xAlign and yAlign, overriding any other values for those options.
  • Make the Chart instance available to positioners. I initially thought I would pass it in as a third parameter; however, I realized it was already available as this._chart, and the position sample uses that. this._chart feels like a private or protected member, and the Scale and LegendElement elements both have a chart member, so I renamed the Tooltip element's _chart to chart to make it consistent and obviously public. I left a _chart reference in place, in case any end-user code followed the sample code, to preserve backward compatibility.
  • Better TypeScript definitions:
    • Add a ...Map, similar to InteractionModeMap, and add documentation on how to keep type safety.
    • Document the this parameter.
  • Update documentation:
    • Custom tooltip positioners seems like a more advanced, more code-heavy feature, so I moved put it at the end of the documentation.
    • Accessing Chart.registry.getPlugin('tooltip') seemed harder than necessary, and it's not what the sample did, and it's not as TypeScript-friendly, so I replaced it with importing Tooltip.
    • Since I was already updating tooltip-related types, I fixed Typescript type for Tooltip custom positioner #9782.

Feedback welcome.

A question: I noticed some inconsistencies in type names:

  • The Legend plugin is called Legend by the TypeScript type definitions. It creates instances of a JavaScript class called Legend, extending Element, whose type is defined in TypeScript as LegendElement, extending Element.
  • Several other types in TypeScript are named ...Element and extend Element.
  • The Tooltip plugin is called Tooltip by the TypeScript type definitions. It creates instances of a JavaScript class called Tooltip, extending Element, whose type is defined in TypeScript as TooltipModel, extending nothing.

The naming differences between JavaScript and TypeScript aren't ideal, but they're consistent. The differences between LegendElement and TooltipModel seem inconsistent, so I wondered about addressing them. I updated TooltipModel to extend Element, but I wondered if it should also be renamed to TooltipElement. That would break backward compatibility for type definitions; it's not too unusual for me to have to tweak my types after minor-version updates, so this may be tolerable for a SemVer-minor release. If you'd like for me to rename TooltipModel to TooltipElement (and optionally make TooltipModel a deprecated alias), either in this PR or another, please let me know.

Fixes #9930.

I initially passed the Chart element as the third parameter to the positioner; however, Scale and LegendElement elements expose `this.chart`, and sample code for positioners used `this._chart`, so documenting the chart member and giving it a public name seems to make more sense.
@joshkel
Copy link
Contributor Author

joshkel commented Dec 3, 2021

Any suggestions on how to resolve the Ubuntu CI test failure? The test suite works when I run it locally on Windows and macOS. I'm having trouble getting a local Ubuntu environment to work.

@kurkle
Copy link
Member

kurkle commented Dec 3, 2021

Any suggestions on how to resolve the Ubuntu CI test failure? The test suite works when I run it locally on Windows and macOS. I'm having trouble getting a local Ubuntu environment to work.

This is a common failure, that I guess has something to do with the load of the action runners. I'll trigger a re-run to see if it fails in the same spot again.

@kurkle
Copy link
Member

kurkle commented Dec 3, 2021

In the re-run windows CI failed, which is even more common (as its slower). You'll need to update the example, so lets see how it goes after that commit.

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed couple more small things

docs/configuration/tooltip.md Outdated Show resolved Hide resolved
src/plugins/plugin.tooltip.js Outdated Show resolved Hide resolved
src/plugins/plugin.tooltip.js Outdated Show resolved Hide resolved
@kurkle kurkle added this to the Version 3.7.0 milestone Dec 5, 2021
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @kurkle's comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltip positioner limitations Typescript type for Tooltip custom positioner
3 participants