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

Fix Bing Maps zoom level, remove map_title template and use Bing Maps API to parse latitude and longitude #17

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hpusset
Copy link

@hpusset hpusset commented Jun 7, 2019

No description provided.

Copy link
Member

@bittner bittner left a comment

Choose a reason for hiding this comment

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

Thank you for your effort!

The linter complains about semicolons, can you take a look?

@@ -1 +0,0 @@
{% if object.title %}<h2>{{ object.title }}</h2>{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason why you remove this here in favor of putting more complexity in the other template?

Copy link
Author

@hpusset hpusset Jun 11, 2019

Choose a reason for hiding this comment

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

I think its unnecessary to create a second template just for one line and append an include to the main template.

Copy link
Member

Choose a reason for hiding this comment

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

Fine, but keep it in a single line, just like in the template.

The idea of the separate template was to allow easy overriding. You may want to wrap the code in a {% block %} now for continued customizability.

CHANGELOG.rst Outdated
@@ -1,6 +1,12 @@
CHANGELOG
=========

1.0.0 (2019-06-07)
Copy link
Member

Choose a reason for hiding this comment

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

Is yours a breaking change that satisfies a major version jump?

I'd prefer to get more of the open issue fixed before releasing a version 1.0.

Copy link
Author

Choose a reason for hiding this comment

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

I change this one to 0.12.0, do you agree ?

Copy link
Member

Choose a reason for hiding this comment

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

Fine.

{% include "djangocms_maps/maps_title.html" %}
{% if object.title %}
<h2>{{ object.title }}</h2>
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

Please, put everything in a single line, no spacing. The HTML output has unpleasant line breaks and inconsistent indenting otherwise.

data-lng="{% if object.lng %}{{ object.lng }}{% endif %}"
{% if object.lat and object.lng %}
data-latlng="{{ object.lat }},{{ object.lng }}"
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid, this will break the implementation of the other map providers.

Also, please make sure the indenting of the resulting output is consistent and there are no superflous line breaks.

Base automatically changed from master to main March 20, 2021 22:30
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.

2 participants