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

chore(examples) Use MapboxOverlay in mapbox (formerly safegraph) website example #8558

Merged
merged 8 commits into from Mar 6, 2024

Conversation

chrisgervang
Copy link
Collaborator

@chrisgervang chrisgervang commented Feb 27, 2024

Closes #8522. For #8557

Change List

  • Replace MapboxLayer with MapboxOverlay
  • Rename website example to mapbox
  • Use react instead of pure-js

Signed-off-by: Chris Gervang <chris@gervang.com>
Comment on lines 82 to 94
arcLayer.setProps({
data: data.filter(d => d.hex === hex)
});
deckOverlay.setProps({
layers: [
poiLayer, // TODO: This was written with an imperative pattern, which won't work for MapboxOverlay. poiLayer needs to be defined.
arcLayer.clone({
data: data.filter(d => d.hex === hex)
})
]
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to hear some opinions on how to demonstrate this kind of partial update in an example. MapboxLayer supported it with a special setProps function added to each layer, but this is inconsistent with the majority of our examples and isn't supported when regular layers are in use, as is the case when using MapboxOverlay.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The same behavior can be done using the MaskExtension: https://github.com/visgl/deck.gl/blob/master/test/apps/mask/app.jsx

If not using that, I think it is OK to recreate all the layers for the setProps call, deck.gl should do the diff internally and only update what is needed

Copy link
Collaborator

Choose a reason for hiding this comment

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

The common way of doing this is to change the signature to renderLayers(deckOverlay, data, selectedPOI) and call it from onClick.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Got it to work.
mapbox-v9

examples/website/safegraph/app.js Outdated Show resolved Hide resolved
Comment on lines 82 to 94
arcLayer.setProps({
data: data.filter(d => d.hex === hex)
});
deckOverlay.setProps({
layers: [
poiLayer, // TODO: This was written with an imperative pattern, which won't work for MapboxOverlay. poiLayer needs to be defined.
arcLayer.clone({
data: data.filter(d => d.hex === hex)
})
]
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

The common way of doing this is to change the signature to renderLayers(deckOverlay, data, selectedPOI) and call it from onClick.

@coveralls
Copy link

coveralls commented Feb 29, 2024

Coverage Status

coverage: 70.671%. remained the same
when pulling fd1c1bb on chr/refactor-safegraph-to-mapbox-overlay
into 5af293f on master.

Signed-off-by: Chris Gervang <chris@gervang.com>
@chrisgervang chrisgervang marked this pull request as ready for review February 29, 2024 23:30
Signed-off-by: Chris Gervang <chris@gervang.com>
@Pessimistress
Copy link
Collaborator

I thought we were renaming this example to mapbox?

@chrisgervang
Copy link
Collaborator Author

@Pessimistress, as discussed, I've switched the example to use react. Lmk if you have any comments or if this is ready to merge.

@chrisgervang chrisgervang changed the title chore(examples) Use MapboxOverlay in safegraph example chore(examples) Use MapboxOverlay in mapbox (formerly safegraph) website example Mar 6, 2024
@chrisgervang chrisgervang merged commit 2e0fd4a into master Mar 6, 2024
3 checks passed
@chrisgervang chrisgervang deleted the chr/refactor-safegraph-to-mapbox-overlay branch March 6, 2024 17:57
@chrisgervang chrisgervang self-assigned this Mar 6, 2024
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.

[Bug] rename safegraph example to mapbox
4 participants