Skip to content

feat(widgets): styled tooltips#10414

Open
ibgreen-openai wants to merge 1 commit into
masterfrom
ib/widget-tooltips
Open

feat(widgets): styled tooltips#10414
ibgreen-openai wants to merge 1 commit into
masterfrom
ib/widget-tooltips

Conversation

@ibgreen-openai

@ibgreen-openai ibgreen-openai commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • Native browser tooltips on widget controls react slowly and cannot use deck.gl themes.
  • Applications that expose keyboard shortcuts need a compact way to show them with the control label.

Goals

  • Render bundled widget labels as responsive, theme-driven tooltips instead of native title tooltips.
  • Keep the implementation in @deck.gl/widgets, with only a small generic hook in core.
  • Preserve accessible names and support one inherited shortcut?: string | HTMLElement prop.

Changes

  • Adds shortcut and the generic onAfterRenderHTML() hook to core Widget.
  • Adds a thin widgets-local base class and delegated tooltip renderer in @deck.gl/widgets.
  • Updates bundled widget controls to emit tooltip metadata and plain aria-label values instead of native title attributes.
  • Adds themed tooltip CSS, shortcut content support, public API docs, release notes, and focused widget coverage.

Screenshots

Before: native browser tooltip After: themed widget tooltip

Validation

  • yarn build
  • yarn lint (passes with existing repository warnings, no errors)
  • ./node_modules/.bin/tsc -p modules/core/tsconfig.json --noEmit
  • ./node_modules/.bin/tsc -p modules/widgets/tsconfig.json --noEmit
  • ./node_modules/.bin/vitest run --project headless test/modules/widgets/widget-tooltip.spec.ts
  • GoogleMapsAPIKey=dummy-google-maps-api-key GoogleMapsMapId=dummy-google-maps-map-id MapboxAccessToken=dummy-mapbox-access-token yarn docusaurus build from website/
  • Full widget suite: tooltip coverage passes; the existing LoadingWidget failure reproduces unchanged on clean origin/master.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7e2261e6dc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread modules/core/src/lib/widget-tooltip.ts Outdated
Comment thread modules/core/src/lib/widget-tooltip.ts Outdated
@coveralls

coveralls commented Jul 3, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 83.142% (+0.007%) from 83.135% — ib/widget-tooltips into master

@ibgreen-openai ibgreen-openai force-pushed the ib/widget-tooltips branch 2 times, most recently from da2fe19 to a8d7aa2 Compare July 3, 2026 14:00
@ibgreen-openai ibgreen-openai added this to the v9.4 milestone Jul 3, 2026

@Pessimistress Pessimistress left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you move this to the widgets module? It doesn't make sense to have a dom component in /core that only works with css in /widgets.

@ibgreen-openai

Copy link
Copy Markdown
Collaborator Author

Addressed in a4ca2b6: the tooltip DOM renderer and thin widget adapter now live in @deck.gl/widgets. Core only retains the inherited shortcut prop and a generic post-render HTML hook; bundled widgets emit tooltip metadata directly.

type ViewOrViews = View | View[] | null;

/** Base class for widgets that render themed HTML controls. */
export abstract class Widget<

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have some concerns about this wrapper approach. I'm wondering how someone implementing a custom widget would take advantage of it?

It's not exported by the module yet, was that intentional?

Having second base Widget class nearly identical to the core one adds an indirection too. If I were a user, how would I know which one to use? The one in core or widgets?

I'd prefer this shortcut and tooltip feature get entirely inlined into the widgets (e.g. no shared interface changes) over this wrapper, or find another mechanism to share the shortcut/tooltip enhancement.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes I am also very unhappy with the dual base class approach, but after I moved this out of core per @Pessimistress request I didn't see an easy way to integrate this cleanly.

Maybe manually implementing it in every widget is better as you suggest.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Custom widgets could still benefit from this being a documented pattern and exporting WidgetTooltip for reuse.

Maybe not ideal, but I think it's a fair tradeoff often made for UI where applications have a long tail of different needs.

We could always revisit this later too if we think of a better method

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants