Skip to content
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

docs(react) React widgets and useWidget hook documentation #9309

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

chrisgervang
Copy link
Collaborator

@chrisgervang chrisgervang commented Dec 19, 2024

For #9278 #9056 #9298

Background

A features not "done" until its documented.. react widget docs.

Change List

  • useWidget api reference
  • JSX widgets usage


const App = (data) => (
<DeckGL
initialViewState={{longitude: -122.45, latitude: 37.78, zoom: 12}}
controller={true}
layers={[new ScatterplotLayer({data})]}
>
<StaticMap
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be good to do an audit for this and use maplibre since it doesn't need a token.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea - Add a bullet to our v9.1 tracker task?

@coveralls
Copy link

coveralls commented Dec 19, 2024

Coverage Status

coverage: 91.705% (-0.002%) from 91.707%
when pulling baaee31 on chr/use-widget-docs
into 05bca74 on master.

@chrisgervang chrisgervang mentioned this pull request Dec 19, 2024
36 tasks

const App = (data) => (
<DeckGL
initialViewState={{longitude: -122.45, latitude: 37.78, zoom: 12}}
controller={true}
layers={[new ScatterplotLayer({data})]}
>
<StaticMap
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea - Add a bullet to our v9.1 tracker task?

docs/api-reference/react/use-widget.md Outdated Show resolved Hide resolved
Comment on lines 7 to 19
import {CompassWidget as VanillaCompassWidget, CompassWidgetProps} from '@deck.gl/widgets';

export const CompassWidget = (props: CompassWidgetProps = {}) => {
const widget = useWidget(VanillaCompassWidget, props);
return null;
};

<DeckGL>
<CompassWidget/>
</DeckGL>
```

See a full example [here](../../developer-guide/custom-widgets/react-widgets.md).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vanilla sounds so weird... And the fact that I can't easily think of a better name makes me feel this is an indication of the dual usage of the word "widget" here being more problematic than initially expected.

How about we reserve the word "widget" for actual widgets, and talk about wrapping widgets into react "components"? I realize it will required rewriting the docs a bit, but I think a ton of confusion can be avoided.

Suggested change
import {CompassWidget as VanillaCompassWidget, CompassWidgetProps} from '@deck.gl/widgets';
export const CompassWidget = (props: CompassWidgetProps = {}) => {
const widget = useWidget(VanillaCompassWidget, props);
return null;
};
<DeckGL>
<CompassWidget/>
</DeckGL>
```
See a full example [here](../../developer-guide/custom-widgets/react-widgets.md).
import {CompassWidget, CompassWidgetProps} from '@deck.gl/widgets';
export type CompassProps = CompassWidgetProps;
export const Compass = (props: CompassProps = {}) => {
const widget = useWidget(CompassWidget, props);
return null;
};
<DeckGL>
<CompassWidget/>
</DeckGL>

See a full example here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a fair point. A Widget is just a hook. You could compose multiple widgets in one component.. widgets may or may not mount a DOM element. So yeah these components could really be anything.. we're just suggesting a usage.

I'll try reworking the PRs with this idea.

The open question I have is how to implement a widget nested in a JSX view..

<MapView id="minimap">
  <Compass />
</MapView>

I was originally going to inject a viewId prop, assuming all props were passed into the hook, or introduce something new like a ViewContext. I've been putting this off thinking the community react library might be a better solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we reserve the word "widget" for actual widgets, and talk about wrapping widgets into react "components"?

I rewrote the developer guide with this framing and I think it is a lot clearer now. I don't think the react examples in the dev guide benefit from using the word "Widget" in their component names, however, I think the deck.gl core widgets wrapped in react should have the same name to their non-react counterpart to avoid confusion.

This naming is inconsistent and will cause confusion:

import {Compass} from '@deck.gl/react';

<DeckGL>
  <ScatterplotLayer/>
  <MapView/>
  <Compass/>
</DeckGL>

I'd prefer the react export be called import {CompassWidget} from '@deck.gl/react'; so that it's clear that it is a widget.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'd prefer the react export be called import {CompassWidget} from '@deck.gl/react'; so that it's clear that it is a widget.

Well, I guess it depends on what one thinks off when using the word "widget". What aspect of the word "Widget" is important to you? If it is basically a React component? Mainly that it needs to be a child of deck? That it supports the positioning prop?

Having different exports with the same name can be confusing down the road, especially when asking people to wrap widgets into react component using useWidget which was my original call-out (the "VanillaCompass" issue).

How about a prefix? ReactCompassWidget? DeckCompassWidget?

Just brainstorming, no strong opinion, your call.

Copy link
Collaborator Author

@chrisgervang chrisgervang Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be to extract a widget class from JSX, like we do with layers and views. It could be done in a way where passing custom react components that use useWidget still works.

Edit: This won't work for TypeScript.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we had a direction in mind that covered all cases (i.e. all the types of objects in the "deck.gl zoo": layers, views, controllers, widgets, ...).

Could the replacement of the current half-broken "layers as JSX" approach also be something like useLayer() and useView()?

Or could there be simple wrapper components instead of hooks?

<Widget widget={CompassWidget} ... />
<Layer layer={ScatterplotLayer} ... />

But I guess for perfect typing custom wrappers need to be spelt out for each wrapper?

const ReactCompassWidget(props: CompassWidgetProps) => 

Or the prop types can be inferred from the component type using some generic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no question to me anymore about dropping the word "Widget" from the dev guide and examples for the custom react-wrapped widgets. The new framing makes more sense to me.

So now it's just a question of what to call (and how to export) the official react-wrapped widgets.

Our JSX implementation, in general, is due for a re-think since it doesn't work with typescript - and we've got a promising alternative in deck.gl-community.

Options at this point:

  • prefix the names to differentiate (+deconflicted name, -a bit clumsy looking)
  • match the name, export from different modules (+consistent naming, -export confusion)

I'll think about this some more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, we were writing at the same time. Interesting idea! That could be a good solution for folks who aren't ready to adopt the custom renderer, but want to use typescript.

We could either wait for deck v10 to deprecate the old version and ship this, or could try adding support for something new before then while maintaining legacy support.

What do you think, @Pessimistress?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could either wait for deck v10 to deprecate the old version and ship this, or could try adding support for something new before then while maintaining legacy support.

  • I think we need to form an idea about what "something new" means first before we can say when it should ship.
  • I personally wouldn't delay 9.1 for this
  • but if we can get some clarity on longer term strategy now, then we can make sure 9.1 is a step in the right direction.

Co-authored-by: Ib Green <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants