-
Notifications
You must be signed in to change notification settings - Fork 56
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
Request for comments #2
base: master
Are you sure you want to change the base?
Conversation
Using an AJAX call we can obtain geolocalized data without saving the model. The admin add a 'geo/' URL to call.
its own settings file.
In this way is avoid the multiple inclusion of the Google API javascript file.
Also modify the Media class.
Hi Gianluca, Thanks for working on this! I'll comment on features now. Media filesCurrently, it is possible to use a custom template with nulled
This doesn't require using custom widget for a form field, and this also doesn't require using a form. I don't want django-easy-maps to be tied to forms, and I really like that it is possible to just drop Currently, the inlined javascript may become an issue if multiple instances of the same map are displayed (because the template is parametrized by map.pk), but this is rarely an issue (and this limitation still apply in your branch). But using Media in admin widget is a good idea. Maybe use an another template while rendering widgets, the one which assumes the external javascript is loaded? It is also better to put easy_maps javascript files to static/easy_maps/js or static/easy_maps, not to static/js because the latter may clash with user files. Settings+1 for defaults in settings, this may be useful. InteractivityThis is a really useful feature, but I think it is better to move work to the client side. Shouldn't we use javascript geocoder instead of geopy/json api in this case ( see https://developers.google.com/maps/documentation/geocoding/#ReverseGeocoding )? I'd prefer the interactivity to be implemented purely in javascript. Inlines+1 Multiple markersI like the idea. Currently easy_maps tag accepts either string or Address instance; is it possible to have multiple markers with just strings or form fields? What should syntax look like? Change listI like the idea. By the way, https://github.com/gipi/django-easy-maps/blob/list-addresses/easy_maps_tests/test_app/models.py is not how django-easy-maps was intended to be used :) The idea was to write
(that's what existing sites probably have) and then use |
Thanks for your reply.
this explains a lot to me and I can say that my intention was to improve the admin-side of this application :) Media FilesIHMO is better to avoid to include the Google's script in the widget and to tell the user to include it in the main template, otherwise we could add an optional version of the tag like
(using a custom template for a common case is annoying).
could be milestoned for a next major version a change in that, maybe indicating it in the documentation, obviously if you are ok with this.
I don't understand what you mean, could you make an example?
ok, I'll commit this InteractivityIf we move the geocoding code client side (hence removing Multiple markers
I don't understand the form fields stuffs, could you elaborate that? BTW since there are some things you are ok with, you could indicate me what commits keep (or modify) so that I can reorganize this branch with only them in it in order to allow you to merge and maybe you could open an issue for each argument in this discussion with the guideline of implementations. |
Hi @gipi and sorry for a long delay! Media filesI like the idea of "with_js" attribute. However there are 3 possibilities: a) include all the js (script tag with a link to Google and our js map initialization code); "with_js" should do (b) - but then it needs a better name because some js would still be included :) Any ideas? I think we shouldn't change the default (with_js=True) though.
I mean passing the same Address instance to several easy_maps tags on the same page. They would have the same pk and js code may become incorrect. I don't know if this is a real-life issue. InteractivityInteractivity wouldn't work without js anyway, would it? Client-side requests make more sense because they provide the same capabilities and don't block the server. I think we should do geocoding (user enters the address) both on server side (when interactivity is not available or the address is hard-coded in template) and on client side (when interactivity is enabled), and we should do reverse geocoding (when user clicks the map) only on client side; the UI is a tricky part for geocoding. I think the implementation may include some hidden fields. Multiple markersI mean "model fields" :) The syntax for static addresses looks good for me. But what with querysets? For example, we have some Shops in database; each shop has an "address" TextField. How to create a map with their addresses on the same map? I think that settings for the default map center are ready (btw, I like |
ok, I'll create a branch of each feature; I think for the end of the week maybe :) |
In this branch I tried to implement some ideas
Media files
Previously the javascript files was included directly in the
easy_maps/map.html
template but when more than one{% easy_map %}
was used, multiple instances of Google maps javascript could create problems (chrome console complains about this).In @1f0a767 I used the
Media
class for the widget used in the rendering of the address field.Settings
Added (@8f71c84 and @bf15559 to be squashed together) a
settings.py
containing the default to be used when a new instance is created (also used to center the map). Together with the changes in @39645fd is possible to avoid the saving of an instance with an empty address.Interactivity
I don't like that for see where the marker will be placed I need to save the instance and come back, so I added some javascript functionality: first of all the
update
button that queries the geolocalization URL (a custom view usingJSON
that responds with the data used to full the form field)This is introduced in @5ca4a70 where a
geo/
URL is added to the Admin url patterns.Also I would like to have the coordinates updated when I move the marker on the map (see @d1a0d5a).
Inlines
An admin class to be used with inlines is added with the same functionalities descripted above, this has required some modification on how functions and
HTML
element classes are named.There is some example models added in @d1a0d5a that use it.
Multiple markers
The
easy_maps
tag is improved so to accept also a list ofAddress
instances that will be placed in the map (see @39645fd).Change list
Since the primary data of this application is geographical data I would like to have a change list page in the admin that contains a map at a zoom level such that all the point are immediately visualized. This (@1755f7d) is a very dirty hack and in future I'd love to implement some interaction between the normal Django table entries and the markers.
Conclusion
This is not code I'm proud of, it's a little messy and should be reorganized but to avoid useless work I need to know what do you think about these ideas.