Skip to content

Bounds #305

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

Merged
merged 7 commits into from
Dec 30, 2015
Merged

Bounds #305

merged 7 commits into from
Dec 30, 2015

Conversation

BibMartin
Copy link
Contributor

Not finished. Adresses #139

else:
return []
else:
raise ValueError('List type expected {!r}'.format(x))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it is better: List type expected. Got {!r}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll do it

@ocefpaf
Copy link
Member

ocefpaf commented Dec 22, 2015

Thanks @BibMartin! I made a few minor comments. Let me know once you are done so I can review it again.

@BibMartin
Copy link
Contributor Author

Thanks @ocefpaf for first review. I'm okay with your comments and will apply them asap.
Before merging, I would like to:

  • add get_bounds call in most tests (see test_topo_json for example)
  • plug back something like Map._auto_bounds that uses this method.

@BibMartin BibMartin added this to the v0.2.0 milestone Dec 22, 2015
@BibMartin
Copy link
Contributor Author

@ocefpaf
I'm finished with this one. After rethinking, I guess it's no need to re-create the _auto_bounds method, provided everyone can create his own bounds in doing :

map.fit_bounds(my_feature.get_bounds())

We can eventually add something in Map.render so that if no FitBounds has been defined, we do

map.fit_bounds(map.get_bounds())

Tell me if you want I add it before squashing.

@BibMartin
Copy link
Contributor Author

@themiurgo I would be interested to have your point of view also ; I realize this addresses #244 in the same time as #139 .

@ocefpaf
Copy link
Member

ocefpaf commented Dec 30, 2015

ocefpaf added a commit that referenced this pull request Dec 30, 2015
@ocefpaf ocefpaf merged commit b987946 into python-visualization:master Dec 30, 2015
@themiurgo
Copy link
Contributor

Sorry, coming late for this. Looks great though! :)

@ocefpaf ocefpaf mentioned this pull request Dec 31, 2015
@ocefpaf ocefpaf added the bug An issue describing unexpected or malicious behaviour label Feb 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue describing unexpected or malicious behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants