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

Add coordinates of mapbox view as a derived property in plotly_relayo… #4413

Merged
merged 9 commits into from
Dec 9, 2019

Conversation

jonmmease
Copy link
Contributor

Closes #4399, following @alexcjohnson 's suggestion from #4399 (comment).

Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

@jonmmease thanks very much for the PR!
Please find my questions below:

test/jasmine/tests/mapbox_test.js Outdated Show resolved Hide resolved
test/jasmine/tests/mapbox_test.js Outdated Show resolved Hide resolved
@etpinard etpinard added this to the v1.52.0 milestone Dec 6, 2019
src/plots/mapbox/mapbox.js Outdated Show resolved Hide resolved
src/plots/mapbox/mapbox.js Outdated Show resolved Hide resolved
src/plots/mapbox/mapbox.js Outdated Show resolved Hide resolved
@jonmmease
Copy link
Contributor Author

Thanks very much for the review @archmoj and @etpinard. I believe I made the requested updates. Anything else?

@jonmmease
Copy link
Contributor Author

Oh, I just noticed that these changes were resulting in a "unrecognized GUI edit" warning on the _derived property when resetting the plot with the modebar button. I added a check to prevent this in cf2c4fc, but let me know if you see a more elegent way to do this.

@jonmmease jonmmease mentioned this pull request Dec 8, 2019
@etpinard
Copy link
Contributor

etpinard commented Dec 9, 2019

Oh, I just noticed that these changes were resulting in a "unrecognized GUI edit" warning on the _derived property when resetting the plot with the modebar button

Ideally, 'mapbox._derived.coordinates[i][j] keys should never slip into the fullLayout._preGUI object. So yeah we could probably do better than your attempt in cf2c4fc.

Looks like you were successful at that on user pan and scroll-zoom: fullLayout._preGUI only contains mapbox center, zoom, bearing and pitch keys after those.

The logic for the modebar "reset" button is here:

for(var i = 0; i < subplotIds.length; i++) {
var id = subplotIds[i];
var subplotObj = fullLayout[id]._subplot;
var viewInitial = subplotObj.viewInitial;
var viewKeys = Object.keys(viewInitial);
for(var j = 0; j < viewKeys.length; j++) {
var key = viewKeys[j];
aObj[id + '.' + key] = viewInitial[key];
}
}

it relies on viewInitial - which does get filled with _derived in your new updateDerived mapbox subplot instance method. So I see two ways of fixing the problem,

  • not stash _dervied values key-values into viewInitial (right? do we really need to stash them?)
  • have the "reset" modebar button handler skip over _ keys in viewInitial

@jonmmease
Copy link
Contributor Author

Thank for taking a look @etpinard, I went ahead and removed _derived from viewInitial since this wasn't needed after all.

@etpinard
Copy link
Contributor

etpinard commented Dec 9, 2019

💃 thanks @jonmmease !!

@jonmmease
Copy link
Contributor Author

Thanks! merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: provide mapbox viewport coordinates in relayout data
3 participants