feat(widgets): styled tooltips#10414
Conversation
There was a problem hiding this comment.
💡 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".
da2fe19 to
a8d7aa2
Compare
Pessimistress
left a comment
There was a problem hiding this comment.
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.
a8d7aa2 to
a4ca2b6
Compare
|
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< |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Problem
Goals
titletooltips.@deck.gl/widgets, with only a small generic hook in core.shortcut?: string | HTMLElementprop.Changes
shortcutand the genericonAfterRenderHTML()hook to coreWidget.@deck.gl/widgets.aria-labelvalues instead of nativetitleattributes.Screenshots
Validation
yarn buildyarn 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.tsGoogleMapsAPIKey=dummy-google-maps-api-key GoogleMapsMapId=dummy-google-maps-map-id MapboxAccessToken=dummy-mapbox-access-token yarn docusaurus buildfromwebsite/LoadingWidgetfailure reproduces unchanged on cleanorigin/master.