-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
POC: PopupWidget #8958
base: master
Are you sure you want to change the base?
POC: PopupWidget #8958
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: Maybe call this a POC rather than an RFC?
Can we articulate the main advantages of PopupWidget
compared to Deck's current tooltip prop?
- Aligning all HTML generating code under widgets seems like one such advantage...
- What else? Theming?
Is this widget strong/complete enough that we would be able to consider deprecating the Deck.tooltip prop in favor of this widget?
Add the doc page for this widget would be a good way to advance this to next stage (once you have made sure there are no vetoes / strong objections).
const position = object.geometry.coordinates; | ||
const text = `${object.properties.name} (${object.properties.abbrev})`; | ||
const style = {width: 200, boxShadow: 'rgba(0, 0, 0, 0.5) 2px 2px 5px'}; | ||
widgets.push(new PopupWidget({position, text, style})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the PopupWidget
just take a callback instead, so we don't need to mess with it like this?
@ibgreen The elephant in the room is definitely the API misalignment with
I'm leaning towards option B. Note that deck is already basically doing this and I've just realized that I've basically reimplemented Tooltip 👀 |
Another point towards C & A is state management. In these, tooltip state is managed by deck for convenience and practicality. I'm pretty sure the python and declarative json environments rely on internally managed state, in particular. I like the unification offered by B), so I think we'd just need a solution for python and weigh the tradeoff of making tooltips harder to get started with by removing the managed version. |
|
The widget manager has an imperative API... could this be made accessible to |
A real "popup" widget will be expected to:
The built-in tooltip in Deck is supposed to offer convenience but only the minimal functionality. While I'm fine with this simple initial implementation of a PopupWidget, it doesn't mean they are the same thing. |
background: 'rgba(255, 255, 255, 0.9)', | ||
padding: 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to use CSS variables and add theming for these. --popup-background
and --popup-padding
could be good here, though I might want to go further and suggest to incorporate other aesthetics like a shadow and inner/outer stroke like the other widget designs. Reason being we can offer an appealing OOB design, and make it easy to customize with vars to match a user's design system.
My approach has been to put styles that aren't 100% necessary for functionality in the stylesheet and theme examples in here.
} | ||
|
||
onViewportChange(viewport) { | ||
this.viewport = viewport; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A broader question since we have eye balls on widgets.. How should we handle if this has been added to no specific viewport, and there are multiple viewports? This remains an unsolved question in all of our viewport-dependent widgets. Currently, only the last viewport in the list will get the popup even if the position
is visible in multiple viewports.
I wonder if we a widget should be able to return multiple UI elements if there are multiple viewports around L66 in onAdd? Or if there should be some warning or documentation to say "you're using multiple viewports, so you should instantiate a widget-per-viewport and supply viewId
"
Background
Working on #8956 got me thinking whether it would be useful to add a
PopupWidget
. We would likely want to provide the same API eventually as ingetTooltip
, I've just implemented thetext
option hereChange List
PopupWidget