plugins: add fullscreen plugin #437
Conversation
|
@sanga this is definitely worth polishing! Nice addition. |
|
|
||
| Note: does not work from within an iframe, and because of this, does not | ||
| work in jupyter notebook | ||
| """ |
There was a problem hiding this comment.
I would put this in the class docstring. Add here just a quick description like Adds fullscreen button to your maps.
|
Addressed all comments (I think). Did a little cleanup. Added (a rather soft) test. Added an example. And did a couple of 'drive by' fixes elsewhere. Let me know if you'd rather have the fixes in a separate branch. Oh and I force pushed with clean git history (let me know if you'd rather I didn't) |
|
And force pushed a few cleanups to the fullscreen test |
| Adds a fullscreen button to your map. | ||
|
|
||
| Note: does not work from within an iframe, and because of this, does not | ||
| work in jupyter notebook |
There was a problem hiding this comment.
You mentioned you filed a bug about this, right? Can you add a cross-reference link here so people can follow the status of that?
|
LGTM. Only 1 minor comment. @BibMartin if you find some time to take a quick look and, this is OK for you, go ahead and merge. Otherwise I will merge this next week once I get back in office. |
|
@sanga do you mind also adding an entry in the changelog file? |
|
So I figured out who was rendering that iframe element (to which the allowfullscreen attr needed to be added). It wasn't jupyter actually. It's branca: python-visualization/branca#13 I've just noticed that I missed your comment about the changelog. Change coming shortly |
Cool. Since that is an in-house issue let's get python-visualization/branca#13 sorted out first before we merge this. I am in a conference and I will take a look at this as soon as I get back. But it looks all good already. Thanks @sanga! |
|
python-visualization/branca#13 is merged so this should be mergable now. |
|
Just used this plugin for a GIS lecture using |
|
no worries! :) |
plugins: add fullscreen plugin
I suppose at this point this is more of a request of interest (or lack of interest).
Tested manually and works. However should have a test added. Also it's somewhat unuseful at the moment (at least for me) as it won't work in an iframe which means that currently (I've filed a bug with the jupyter guys) it won't work in jupyter.
But, basically. Should I bother polishing this a bit more (adding a test etc)?