-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(core): Add container to WidgetProps
#9922
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
Conversation
chrisgervang
left a comment
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.
Lgtm! I like how the manager handles the custom container as opposed to the widget itself needing to be concerned with which element it's appended to.
What about CSS? It's possible some selectors weren't written with an external container in mind, and the user will need to add some classes to their container
Each widget's root element (attached to the external container in this case) has the |
| #### `container` (string | HTMLDivElement, optional) {#container} | ||
|
|
||
| The container that this widget is being attached to. Default to `viewId`. | ||
| - If set to `'root'`, the widget is placed relative to the whole deck.gl canvas. |
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.
Considering API options:
- Seems to me that
rootcould also be a validplacement, which would then ignore viewId?
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.
Placement doesn't have any interactions with viewId in implementation, it just positions within a view (or root) box. Perhaps the name is a little confusing.. "alignment" would have been a great name since that values are like "top-left", except that "fill" is an option.
If we're open to a breaking change, "alignment" could be more clear to new users
| The container that this widget is being attached to. Default to `viewId`. | ||
| - If set to `'root'`, the widget is placed relative to the whole deck.gl canvas. | ||
| - If set to a valid view id, the widget is placed relative to that view. | ||
| - If set to a HTMLElement, `placement` is ignored and the widget is appended into the given element. |
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.
Is the goal to make widgets usable outside the deck.gl canvas?
While useful, are there "scope creep" risk with this? There was a lot of concern about this early on, we did not set out to build a generic UI system and make ourselves a target for a "never ending list of corner-cases".
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.
Valid point.. I think this external container idea aligns with early widget visions that included making sidebar panels.
Still, the unknown implications are my main concern as well. I'd be open to marking container as an experimental API to see how it works in practice before locking it in. Wdyt?
It'd be great to see basic examples in purejs and react to validate this concept is a good idea
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.
#8057 - while CARTO isn't actively pushing this use case, I can see others (and myself) having the use case. In a sense, a widget is an accessor of deck's state and update lifecycle
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, agree it is extremely useful. However, there is likely a "long tail" when starting to integrate into an external DOM.s
- I feel we have an opportunity to be deliberate about this (i.e. update the Widget roadmap / vision / deck and also our documentation) to set some expectations / goals for this.
- Generally, we made a lot of progress on widgets in 2025 and I think an updated "Widgets 2026 roadmap" would be great. We still have the widgets working group channel etc in place.
But as you say, if time does not permit, we can of course always slap it in as an experimental _container prop just to get it in place for now.
60354c9 to
71f9b93
Compare
71f9b93 to
3e2d35d
Compare
Background
widget.viewIdis currently overloaded with two purposes:The above two are often the same, but not always. This PR proposes a new prop so that the user can specify the two separately. When not provided,
containerfalls back toviewIdwhich is consistent with the existing behavior.Change List
containerto WidgetProps