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

feat: first iteration to implement zoom level persistence in DB #91

Closed

Conversation

vladox
Copy link

@vladox vladox commented Sep 29, 2021

Also implements hide_latlng for streamfields
Fixes #85

@marteinn
Copy link
Member

marteinn commented Jan 4, 2022

Hi @vladox and thanks for the PR! Sadly I cannot accept it because of:

  • The string format SRID=5432;POINT(12.0 13.0) is a string representation of a geo point and is done by Django/Postgres, extending upon it to include additional values would break compability
  • The zoom feature only works for non-spatial fields, this is due to the zoom being stored on the geo point string representation. This kind of feature would need to work on both spatial and non-spatial fields.

Widgets in Django/Wagtail are a bit tricky to work with, but I think a better approach would be to use a hidden input field that handles the zoom value (we have a similar approach for the location search field). Another way would be to introduce some sort of meta field (for values not related to location), this field could be of a json type and contain zoom and other arbitrary values.

Apologies for not accepting this PR, it's always a good idea to contact the maintainer if you plan on building a larger feature :)

When it comes to implementing hide_latlng I think it's a good idea! Will look through your PR and see if I can make a commit out of it. Will keep this PR open until I have done this.

/ M

@marteinn
Copy link
Member

marteinn commented Jan 4, 2022

hide_latlng is now implemented in GeoBlock. 0c0a3c6. Thanks for the help @vladox!

When it comes to the other part of the PR (persisting zoom), I'm closing this with my reasoning on the comment above, but would be open to review a PR with another approach!

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.

Set zoom level dynamically from a GeoPanel
2 participants