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
website: Upgrade to Mapbox v3 #8600
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.
Approving since this PR does strictly improve the example, but I think there is an issue with renderToDOM
as currently implemented on master
.
@@ -43,7 +43,7 @@ class MapboxDemo extends Component { | |||
} | |||
|
|||
componentDidUpdate(prevProps) { | |||
if (this.props.data && !prevProps.data) { | |||
if (this.props.data && !prevProps.data && this._view) { |
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 looks like renderToDOM
never returns anything ...
... so this._view
is never defined? I don't know if it should block this PR, but something seems broken here.
Related:
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 thought renderToDOM
is only used when running the example standalone (see index.html in the example's folder), whereas the website imports App
directly.
Edit: Oh, nevermind. It mounts the example as a react root within our react website using renderToDOM.. that might be fine, but I see a bug in the signatures. I dropped the data
parameter when I ported it. I'll take a look today
For #8582 fixes https://felixpalmer.github.io/deck.gl/examples/mapbox
Change List
_view
is uninitialized incomponentDidUpdate