-
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
Add widgets prop to Deck class #8023
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.
The list needs to be deep compared in every setProps
call. See implementation of the EffectManager.
fc6be2c
to
f783461
Compare
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.
Thanks @Pessimistress for adding the declarative and imperative functions. Are there any caveats? Also, what do you think about adding a removeDefault
with this design?
for (const id in this.containers) { | ||
this.containers[id].remove(); | ||
} | ||
} | ||
|
||
add( | ||
// Imperative API, not affected by the declarative prop | ||
addDefault( |
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.
Can an imperative user call this to add a widget in their application?
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.
It's not exposed right now, we need an overall strategy for imperative APIs (layers, views, effects, widgets).
b00ae70
to
46be41a
Compare
this.id = props.id; | ||
this.viewId = props.viewId || null; | ||
this.placement = props.placement || 'top-left'; |
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 see, weather or not to include these members as props would be case-by-case. For instance, doesn't make sense for tooltip to have a placement prop.
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.
Not sure if the meaning of these constants can be extended, but tooltips could be positioned relative to the cursor.
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.
The placement for that use-case would still be "fill" I imagine, since it can still be anywhere in the map container. Marker would use the same placement, and also wouldn't expose container placement.
I'd imagine positioning relative to cursor would be internal to the widget.
widgetManager.add(widgetB, {viewId: 'map', placement: 'bottom-right'}); | ||
t.is(widgetManager.widgets.length, 2, 'widget is added'); | ||
t.is(widgetB._viewId, 'map', 'view id is assigned'); | ||
const widgetB = new TestWidget({id: 'B', viewId: 'map', placement: 'bottom-right'}); |
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 like this API change. Having placement and viewId on add
didn't make sense.
@@ -988,7 +992,7 @@ export default class Deck { | |||
deck: this, | |||
parentElement: this.canvas?.parentElement | |||
}); | |||
this.widgetManager.add(new Tooltip(), {placement: 'fill'}); | |||
this.widgetManager.addDefault(new Tooltip()); |
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.
Nit: Is there any way to avoid addDefault()
? Such "global" registration is usually best avoided if possible. Can the tooltip be added when it is actually used?
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.
We'd want to consider if changing it could break something since the 8.9 and prior always adds a tooltip to the container.
lastViewports: {[id: string]: Viewport} = {}; | ||
|
||
/** Widgets added via the imperative API */ | ||
private defaultWidgets: Widget[] = []; |
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.
Maybe the names could be more reflective of the comments - imperativeWidgets
/ declarativeWidgets
/ allWidgets
etc?
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.
Yes maybe.. there's ongoing discussion in a new RFC on what our imperative API could be.
// widget already added | ||
return; | ||
/** Imperative API. Widgets added this way are not affected by the declarative prop. */ | ||
addDefault(widget: Widget) { |
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 was about to suggest a different name, but I guess this was renamed to addDefault based on previous comments.
this.id = props.id; | ||
this.viewId = props.viewId || null; | ||
this.placement = props.placement || 'top-left'; |
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.
Not sure if the meaning of these constants can be extended, but tooltips could be positioned relative to the cursor.
--------- Co-authored-by: Xiaoji Chen <cxiaoji@gmail.com>
For #7946
Background
To build any widgets we'll need some interface in Deck to add them. This adds a declarative interfaces to the Deck API for applications to add/update/remove widgets.
Note: This PR adds a private imperative "addDefault" similar to
EffectManager
so that the tooltip widget isn't affected by an application. There is ongoing discussion around a public imperative interface.Change List
widgets
prop to Deck for declarative use cases.useDefault
function toWidgetManager
to maintain tooltip functionalityplacement
andviewId
as widget members.