New bucket for tooltips, also added tests, and updated packages#268
New bucket for tooltips, also added tests, and updated packages#268v-aidaba wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a dedicated Tooltips data role/bucket for the StreamGraph visual and updates tooltip generation/binding so tooltips can be resolved per hovered stream point. It also bumps the visual version and updates dependencies, with new unit tests covering tooltip behavior.
Changes:
- Added a new Tooltips data role + mapping support (and capability metadata for tooltip support).
- Updated tooltip generation to support “explicit tooltips” (only Tooltip bucket fields when populated) and per-point tooltip resolution.
- Added unit tests for tooltip fallback vs explicit tooltip bucket output, plus version/dependency updates.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/visual.ts |
Builds per-point tooltip info during conversion and binds pointer-based tooltip events. |
src/tooltipBuilder.ts |
Implements explicit-tooltip-column detection and tooltip item construction logic. |
capabilities.json |
Adds the Tooltips role, updates dataViewMappings to include it, and declares tooltip support. |
test/visualTest.ts |
Adds tests validating default tooltip fallback and explicit tooltip bucket behavior. |
stringResources/en-US/resources.resjson |
Adds Visual_Tooltips string for the new role label. |
src/dataInterfaces.ts |
Extends StreamDataPoint with optional tooltipInfo. |
pbiviz.json |
Bumps visual version/displayName to 3.3.0.0. |
package.json / package-lock.json |
Dependency updates (including a powerbi-visuals-api version change). |
CHANGELOG.md |
Documents new tooltip bucket behavior + tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,3 +1,14 @@ | |||
|
|
|||
There was a problem hiding this comment.
There’s an extra leading blank line at the very top of the changelog entry, which makes the file start with an empty line. Removing it keeps the markdown formatting consistent and avoids odd rendering in some viewers.
| // Tooltip binding extracted (same logic, cleaner update) | ||
| this.bindLayerTooltips(selection as any, this.data); |
There was a problem hiding this comment.
The tooltip handling was switched to use visualHost.tooltipService directly, but tooltipServiceWrapper is still created/stored and is now unused. Either remove the wrapper entirely (property + initialization + import) or keep using tooltipServiceWrapper.addTooltip(...) to avoid dead code and keep tooltip behavior consistent with the utils wrapper.
| // Tooltip binding extracted (same logic, cleaner update) | |
| this.bindLayerTooltips(selection as any, this.data); | |
| // Tooltip binding: use the tooltipServiceWrapper to attach tooltips to the rendered layers | |
| if (this.tooltipServiceWrapper) { | |
| this.tooltipServiceWrapper.addTooltip( | |
| selection as any, | |
| (tooltipEvent: any) => this.getTooltipInfo(tooltipEvent.data, this.data), | |
| (dataPoint: any) => dataPoint.identity, | |
| true | |
| ); | |
| } |
There was a problem hiding this comment.
Removed tooltipServiceWrapper since it is no longer used. In this case, it was rendering the tooltip in a stream-specific way and did not allow per-point tooltip handling, so I switched to using visualHost.tooltipService directly.
| private hideTooltip(): void { | ||
| this.visualHost.tooltipService.hide({ isTouchEvent: false, immediately: false }); | ||
| } |
There was a problem hiding this comment.
hideTooltip() always calls tooltipService.hide with isTouchEvent: false. On touch interactions (where showOrMoveTooltip sets isTouchEvent based on event.pointerType), this can prevent the touch tooltip from being dismissed correctly. Consider passing through the pointer type (e.g., accept the triggering event in hideTooltip or track the last isTouchEvent value).
There was a problem hiding this comment.
Updated the implementation to track the last isTouchEvent value, so hideTooltip() uses the correct interaction type as well.
| // Create tooltip info for this specific data point | ||
| const dataPointTooltipInfo: VisualTooltipDataItem[] = createTooltipInfo( | ||
| dataView, | ||
| { categories, values }, | ||
| localizationManager, | ||
| valueIndex, | ||
| dataPointValueIndex | ||
| ); |
There was a problem hiding this comment.
Tooltip info is now being built for every data point during conversion (createTooltipInfo inside the inner values loop). For large datasets this is potentially expensive (formatting + role filtering per point). Consider computing tooltip items lazily on hover (or caching per column/index) so conversion remains O(n) without heavy per-point formatting.
There was a problem hiding this comment.
Added caching for tooltip generation to avoid recomputing the same formatting work for each data point.
| export function createTooltipInfo( | ||
| dataView: DataView, | ||
| dataViewCat: DataViewCategorical, | ||
| localizationManager: ILocalizationManager, | ||
| seriesIndex?: number): VisualTooltipDataItem[] { | ||
| seriesIndex?: number, | ||
| categoryIndex?: number | ||
| ): VisualTooltipDataItem[] { |
There was a problem hiding this comment.
dataView is no longer used by createTooltipInfo/createTooltipData (it’s only forwarded), which triggers the repo’s @typescript-eslint/no-unused-vars rule for unused parameters. Either remove this parameter from both functions (and update call sites) or rename it to _dataView to explicitly mark it as intentionally unused.
There was a problem hiding this comment.
Removed it , dataView is no longer used
| "Visual_TitleText": "Title text", | ||
| "Visual_Tooltips": "Tooltips", | ||
| "Visual_Tooltips_Highlighted": "Highlighted", |
There was a problem hiding this comment.
A new localization key Visual_Tooltips was added for en-US only. Other locales under stringResources/*/resources.resjson will be missing this key, which can cause the tooltip role label to fall back to the raw key or show blank in non-English locales. Please add the key to the other locale resource files (or implement a fallback strategy if that’s the project convention).
There was a problem hiding this comment.
This key was added intentionally only for en-US. The other locale files are not required at this stage, since the remaining translations are handled later
| "powerbi-models": "^2.0.1", | ||
| "powerbi-visuals-api": "5.11.0", | ||
| "powerbi-visuals-utils-chartutils": "^8.1.0", | ||
| "powerbi-visuals-api": "~5.7.0", |
There was a problem hiding this comment.
powerbi-visuals-api was changed from 5.11.0 to ~5.7.0, which is a downgrade (and also changes the pinning strategy). This can introduce API/type mismatches with powerbi-visuals-utils-* packages that depend on newer API versions and may result in multiple powerbi-visuals-api versions in the lockfile. Please confirm this downgrade is intentional; otherwise keep the API version aligned/up-to-date.
| "powerbi-visuals-api": "~5.7.0", | |
| "powerbi-visuals-api": "5.11.0", |
There was a problem hiding this comment.
Updated it and aligned the package version to the newer one
| return hasExplicitTooltipFields ? [] : seriesData.tooltipInfo || []; | ||
| } | ||
|
|
||
| // Compute tooltip info lazily on first access |
There was a problem hiding this comment.
Am I right about following observation? In default mode (Tooltips bucket is empty), createTooltipInfo ignores categoryIndex - it builds the tooltip purely from the series column. Every data point of the same series produces identical output. However, the cache key is ${seriesIndex}-${pointIndex}, which creates N duplicate entries per series (one per hovered point).
For a dataset with 5 series × 365 points, the cache grows to 1825 entries instead of 5.
If I didn't missed something that could improve this:
// In default mode all points of the same series produce identical items,
// so key by series only. In explicit mode each point can differ.
const cacheKey = hasExplicitTooltipFields
? `${seriesIndex}-${pointIndex}`
: `${seriesIndex}`;
|
|
||
| private hasExplicitTooltipFields(dataView: DataView): boolean { | ||
| const categoricalData: DataViewCategorical | undefined = dataView?.categorical; | ||
| const hasTooltipsRole = (roles?: { [key: string]: boolean }): boolean => !!(roles && roles["Tooltips"]); |
There was a problem hiding this comment.
TooltipsRoleName is declared in tooltipBuilder.ts as a module-private const, so hasExplicitTooltipFields() in visual.ts independently hardcodes the same string "Tooltips". If the role name changes, one place will be missed silently.
Export TooltipsRoleName from tooltipBuilder.ts and:
// visual.ts
- import { createTooltipInfo } from "./tooltipBuilder";
+ import { createTooltipInfo, TooltipsRoleName } from "./tooltipBuilder";
// hasExplicitTooltipFields():
- const hasTooltipsRole = (roles?: ...) => !!(roles && roles["Tooltips"]);
+ const hasTooltipsRole = (roles?: ...) => !!(roles && roles[TooltipsRoleName]);
| "powerbi-visuals-utils-formattingutils": "^6.1.2", | ||
| "powerbi-visuals-utils-interactivityutils": "^6.0.4", | ||
| "powerbi-visuals-utils-svgutils": "^6.0.4", | ||
| "powerbi-visuals-utils-tooltiputils": "^6.0.4", |
There was a problem hiding this comment.
so we don't plan to use powerbi-visuals-utils-tooltiputils anymore?
No description provided.