feat(react) Support for JSX Widgets in React #9278
Conversation
Signed-off-by: Chris Gervang <[email protected]>
Signed-off-by: Chris Gervang <[email protected]>
Signed-off-by: Chris Gervang <[email protected]>
Signed-off-by: Chris Gervang <[email protected]>
Signed-off-by: Chris Gervang <[email protected]>
Signed-off-by: Chris Gervang <[email protected]>
Signed-off-by: Chris Gervang <[email protected]>
There was a problem hiding this comment.
Looks good to me, clean and not too much code :) I'm not that familiar with the React module so will leave the approval to @Pessimistress
modules/widgets/src/zoom-widget.tsx
Outdated
|
|
||
| this.props = { | ||
| ...props, | ||
| transitionDuration: props.transitionDuration || 200, |
There was a problem hiding this comment.
Wonder if it would be nice to support defaultProps for widgets to avoid this sort of code
There was a problem hiding this comment.
Widget is only an interface, so I think the best we can do is lead by example of least annoying code to maintain. I just changed the dev guide to use this.id = props.id ?? this.id to avoid needing to repeat the hard-coded values, which is a marked improvement over this.
There was a problem hiding this comment.
Could it be an abstract class instead?
There was a problem hiding this comment.
It's tempting but I'm more in favor of flexibility at this stage. I'm hesitant to abstract too early across widgets with there are so many ways to implement UI.
It's a bit of extra work to fill out the interface, but not much.
Maybe when we have a fuller library of examples exploring a breadth of use cases.. until then, there's nothing preventing an author from making their own abstract class.
| @@ -8,7 +8,7 @@ import type {Deck, Viewport, Widget, WidgetPlacement} from '@deck.gl/core'; | |||
| import {render} from 'preact'; | |||
| import {ButtonGroup, GroupedIconButton} from './components'; | |||
|
|
|||
| interface ZoomWidgetProps { | |||
| export interface ZoomWidgetProps { | |||
There was a problem hiding this comment.
@Pessimistress I'm noticing that the widget's onViewportChange isn't called on initialization. The user needs to interact with deck before a viewport is set.
Vanilla widgets work. Any idea what's different?
There was a problem hiding this comment.
The cause is this guard is hit before the viewports have been assigned to the widget. lastViewports was set on the first time props are set (when widgets: [] and it initialized the tooltip).
The goal of this code is to cache the current viewport so that the widget can use it as the starting point of its view modification.
The hook adds widgets in one-by-one: [] then [FirstWidget] then [FirstWidget, SecondWidget]
There was a problem hiding this comment.
I guess we could push this check down into each widget to implement since the manager isn't aware of how widgets store viewports, or we could add viewports: {} to class Widget and have the manager check that instead? Thoughts?
There was a problem hiding this comment.
See proposed fix in #9303
chrisgervang
changed the title
| output: { | ||
| children: [], | ||
| views: null, | ||
| layers: null |
There was a problem hiding this comment.
Adding this test case since it was unclear that layers aren't always returning an empty array, regardless of input.
| @@ -64,27 +65,40 @@ test('positionChildrenUnderViews#before initialization', t => { | |||
| test('positionChildrenUnderViews', t => { | |||
There was a problem hiding this comment.
deck children are now wrapped in a DeckContext.Provider by default
| t.is( | ||
| layers && Array.isArray(layers) && layers.length, | ||
| 0, | ||
| 'Layers is reset to an empty array' | ||
| ); |
There was a problem hiding this comment.
This layers prop behavior is subtly different than pure-js and so I wanted to capture it in our tests. Widgets match this behavior as well.
| onAfterRender: () => { | ||
| const {widgets, layers} = deck.props; | ||
| t.is(widgets && Array.isArray(widgets) && widgets.length, 1, 'Widgets remain set'); | ||
| t.is(layers && Array.isArray(layers) && layers.length, 1, 'Layers remain set'); |
There was a problem hiding this comment.
See below, this works a little differently between react and pure-js. I believe this is intentional and just wasn't covered in our tests.
| @@ -157,6 +157,7 @@ function DeckGLWithRef<ViewsT extends ViewOrViews = null>( | |||
| // Needs to be called both from initial mount, and when new props are received | |||
| const deckProps = useMemo(() => { | |||
| const forwardProps: DeckProps<ViewsT> = { | |||
| widgets: [], | |||
There was a problem hiding this comment.
Similar to jsxProps.layers, the widgets array needs to reset to an empty array in the react component so that widgets unmount when the prop is removed.
This sets the behavior when a user goes from <DeckGL widget={[...]}/> to <DeckGL/>.
I believe unmounting in this case is the desired behavior. This behavior was noticeable in the deck.gl playground (which uses the react deck component) when removing the entire widgets section from the json string didn't remove them from the map.
For #9056
Related #9304 #9309
Background
Change List