Skip to content

Add cluster features to scattermapbox trace#5147

Closed
elben10 wants to merge 672 commits into
plotly:masterfrom
elben10:master
Closed

Add cluster features to scattermapbox trace#5147
elben10 wants to merge 672 commits into
plotly:masterfrom
elben10:master

Conversation

@elben10

@elben10 elben10 commented Sep 14, 2020

Copy link
Copy Markdown
Contributor

I've added a trace that allow one to use mapbox clustering layer.

Currently it doesn't support the fill, selection and text features.

@archmoj archmoj added status: reviewable community community contribution labels Sep 14, 2020
@archmoj

archmoj commented Sep 14, 2020

Copy link
Copy Markdown
Contributor

@elben10 Thanks for the PR!
Could you please pick this commit d9c6e02 that fixes the mapbox filename, adds the baseline and some tests.

Comment thread src/traces/scatterclustermapbox/attributes.js Outdated
Comment thread src/traces/scatterclustermapbox/convert.js Outdated
Comment thread src/traces/scatterclustermapbox/attributes.js Outdated
@alexcjohnson

Copy link
Copy Markdown
Collaborator

@elben10 thanks for the PR, it looks really useful! But let's take a step back before going further with it - does this really need to be a separate trace type? It seems to me as though, in terms of the attributes involved, it's just a few extra options on top of scattermapbox to control the clustering, and the attributes regarding lines are not allowed. I'd suggest adding a cluster.enabled attribute, which defaults to true whenever any of the other cluster attributes is set explicitly; then when cluster.enabled===true the only mode we will allow is markers and the line attributes will automatically not be used. Then in the following steps we can just look at the cluster.enabled attribute to determine which pathway to take.

Does that seem like a reasonable way to include this feature?

@elben10

elben10 commented Sep 14, 2020

Copy link
Copy Markdown
Contributor Author

@alexcjohnson I think that sounds like a better choice. I'll update the code accordingly

@archmoj archmoj added feature something new and removed type: new trace type labels Sep 14, 2020
@elben10 elben10 marked this pull request as draft September 14, 2020 21:20
Comment thread test/jasmine/assets/mock_lists.js Outdated
@elben10 elben10 marked this pull request as ready for review September 15, 2020 09:45
Comment thread src/traces/scattermapbox/plot.js Outdated
@elben10 elben10 marked this pull request as draft September 22, 2020 05:37
Comment thread src/traces/scattermapbox/convert.js Outdated
Comment thread src/traces/scattermapbox/convert.js Outdated
Comment thread src/traces/scattermapbox/convert.js
Comment thread src/traces/scattermapbox/plot.js Outdated
Comment thread src/traces/scattermapbox/attributes.js
Comment thread src/traces/scattermapbox/attributes.js Outdated
Comment thread src/traces/scattermapbox/defaults.js Outdated
Comment thread src/traces/scattermapbox/plot.js
Comment thread src/traces/scattermapbox/defaults.js Outdated
Comment thread src/traces/scattermapbox/hover.js Outdated
Comment thread src/traces/scattermapbox/convert.js Outdated
Comment thread src/traces/scattermapbox/plot.js
Comment thread src/traces/scattermapbox/plot.js Outdated
Comment thread test/image/mocks/mapbox_scattercluster.json Outdated
@archmoj

archmoj commented Dec 6, 2020

Copy link
Copy Markdown
Contributor

337 file changes! This is now extremely hard to review.
Could you start a fresh PR and rebase/squash the changes you made to mapbox only on top of v1.58.1?
Please also consider giving us as the maintainers permission to edit as described in the PR template.
Thank you!

@elben10 elben10 closed this Dec 6, 2020
@archmoj

archmoj commented Dec 6, 2020

Copy link
Copy Markdown
Contributor

@elben10
To make it easier for you to submit a new PR I fixed the merge conflicts on this temporary branch:
https://github.com/plotly/plotly.js/compare/cluster-scattermapbox

To open the final PR you may consider starting a new fork.
Then

git checkout cluster-scattermapbox
git reset HEAD^
git add .
git commit 

So that you get credit for the great contribution you made.
Then

git push -f

Then please open a PR from your cluster-scattermapbox branch to our master.

Does that make sense?

@elben10 elben10 mentioned this pull request Dec 10, 2020
3 tasks
@elben10

elben10 commented Dec 10, 2020

Copy link
Copy Markdown
Contributor Author

Yes it does. The pull request is now created! Thanks for the help and guidance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community community contribution feature something new

Projects

None yet

Development

Successfully merging this pull request may close these issues.