EZP-28647: Difference in edit and preview mode for map location #2236

mikadamczyk · 2018-01-26T13:53:43Z

JIRA: https://jira.ez.no/browse/EZP-28647

Description

When we set latitude and longitude to '0' we should see proper information, not "Not set" message.
Additionally when we did not provide values for latitude and longitude, we still should see a map on the frontend page, with zoom level 1 and coordinates equal 0,0. This PR also changed google maps to openStreetMaps.

gggeek · 2018-01-26T16:18:26Z

I think that the proper handling of NULL vs 0 values should not go together with the switch from gmaps to openstreetmap.
Also, the latter should at least be subject to a config file, and still default to Google, for BC.

crevillo · 2018-01-26T16:40:12Z

Agree with @gggeek here.

lserwatka · 2018-01-27T08:53:23Z

@gggeek @crevillo At the product management we decided to drop Google Maps support for Open Source version of the Product in favour of Open Street Maps. For consistency with new admin UI introduced in 2.0.0 version we have to change it too here. This PR should go to master (kernel 7.1) as it might be unexpected change for .dot release for those who are already started using it. This template has been a bit outdated and we will do more cleanups bringing it up to date, likely BC will happen in version 2.1.0. We will document all possible BCs in the official release note.

crevillo · 2018-01-27T09:02:07Z

Dunno.... i'd say this dont fit semantic versioning principles.

lserwatka · 2018-01-27T09:11:47Z

It is just a principle, we are at the beginning of .0 for 2.x series and we have to make this cleanup. I fully understand BCs might be painful, but it is temporary pain. But we will document it. Last composer change broke literally all open source projects due to licensing change in .dot release. Things like that will happen. And that did not fit semantic versioning.

crevillo · 2018-01-27T09:14:54Z

Ok. Probably easy to understand for me, not that easy for possible customers.

emodric · 2018-01-27T10:04:40Z

I also agree with @gggeek, there should be some kind of BC layer here for existing users. eZ always took pride in its BC policy, so this feels kinda counterproductive. What's wrong in making it configurable?

Will the old version of the template be provided for those who do not want to switch?

The argument that others do it, so we can too is wrong in my opinion. That's not how semver should work.

lserwatka · 2018-01-27T10:07:51Z

@crevillo current inconsistent behaviour will do a lot more damaged to developers and customers expectations where admin UI got Open Street Map and frontend got Google Maps. This PR fixes it.

lserwatka · 2018-01-27T10:16:38Z

@emodric Netgen Tags introduced BC break be changing param name from eztags to netgen tags prefix, without informing anyone about, that broken ezplatform.com for instance.

We are not using argument that others doing it like that. Main argument is maintenance cost that someone need to take at some point.

emodric · 2018-01-27T10:23:04Z

That change happened in version 3.0 I think, where BC break is allowed, and it was documented in release notes. If I'm wrong and if it happened in minor release, it should've been reported :)

The problem with this change is that it directly ties into that the frontend will change in most cases on customers' sites after the upgrade, which is far more important than an inconsistency in admin.

lserwatka · 2018-01-27T10:34:51Z

@emodric I'm afraid change happened in one of minor versions.

We will document this change as part of upgrade process.

emodric · 2018-01-27T10:37:07Z

I am sorry about that then, it should've been reported :)

Documenting it is not enough maybe. In my opinion, you should provide a way for people to revert back to Google Maps if they wish.

crevillo · 2018-01-27T10:45:46Z

What @emodric says is exactly my point. one of the goodness of ez is that you can use ezpublish-kernel without (any) admin bundle. And that's exactly what we're trying in some current projects and what we are offering to some others possible customers. So, i don't think what happened in the admin bundle should introduce a BC change in the kernel.

That said, if finally this goes in, we could remove this line, right? https://github.com/ezsystems/ezpublish-kernel/pull/2236/files#diff-c8b22ddd1af7311bccfea99ccb809930R359

crevillo · 2018-01-27T11:09:58Z

Finally on my side

we are still calling the block ezgmap_location. I think we should change that in case this is not a google map anymore
Another possible solution which i'd accept is to override that block in the admin bundle. Similar to what is done in demos with the image fields. https://github.com/ezsystems/ezplatform-demo/blob/master/app/Resources/views/themes/demo/content_fields.html.twig

lserwatka · 2018-01-27T16:33:05Z

We will provide detailed documentation and code snippet for TWIG template to make upgrade process easier by extending kernel template. Most of site implementations are doing this anyway. So the effort will lead towards copy & paste code and placing it in a right place.
Proposed change in this PR touches only UI (presentation layer), does not introduce any API or FieldType PHP implementation backward compatibility breaks.
We will retain gmapl_location FieldType name, as stands for GeoMap Location (provides Map, and geo coordinates), and implementation on PHP level does not require any changes. It is UI layer which might use Open Street Map or Google Maps or what ever is needed. It is up to integrator. We as a vendor will relay on Open Street Map.
We don't have intention to maintain two UI layers for Open Street Map and Google Maps.
Kernel is a key component for our eZ Platform and eZ Platform EE products and must provide up-to-date feature set used on higher levels of the system.

With point #1 I strongly believe upgrade path won't be difficult and time consuming. It is only presentation layer after all. Thank you for your all feedback and understanding.

andrerom · 2018-01-29T11:11:37Z

@lserwatka / et al: I think some of the differences here comes from some thinking we follow Symfony's BC promise, and assuming this template code is per SemVer an API, which per SemVer implies increasing major version bump on changes.

We don't necessarily follow Symfony on everything, they are making something a bit different then we do, however we are overdue for clarifying what we follow as SemVer has lots of grey area.

Problem with trying to force us to follow Symfony on everything; this is not the only place we have a need for cleanup, some of the others:

Move legacy storage engine out of kernel (to be able to introduce preview of improved storage engine)
Move Page field type out of kernel (just long overdue)
Remove HttpCache implementation in kernel [7.1] Remove Foshttpcache code, leaving that to ezplatform-http-cache #2194 (in order to be able to move on to FOSHttpCache 2.x and be able to start testing with Symfony 4.x)
... overnights we did not have time, or where not aware of during initial v2 release..

To avoid us being forced to do a major release on every release like browser vendors do, a compromise to allow continued innovation would perhaps be something along:

Start process to clarify what API is in eZ, acknowledge more then Public API is counted as API in SemVer term (Public API has a BC promise across major versions btw, so there is a difference, clarify that to)
Clarify that we sometimes will cleanup code in minors that has been previously been deprecated before/on current major (something deprecated in 2.0.0 or before will thus be allowed to be fixed in 2.x.0)
Allow for moving code out of one package as long as it becomes available in another in a compatible manner
Allow for some slack on cleanups first 1-2 minors just after a major in regards to smaller oversights and inconsistencies

I have a draft blog post from end of last year I can try to finish if anyone would like to review and discuss this. For instance if we do a first round @lserwatka and then I do some further rounds with community maintaners to align on this would that be ok for all?

andrerom · 2018-01-29T11:16:00Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/content_fields.html.twig

        <script>
            if (typeof(window.ezgmaplocationMapsScriptLoaded) == 'undefined') {
                (function (win, doc) {
                    var myScript = document.createElement('script');
-                    myScript.src = '//maps.googleapis.com/maps/api/js{% if apiKey %}?key={{ apiKey }}{% endif %}';


If this is not used anymore we should deprecate the setting (including describing that it is not used anymore)

emodric · 2018-01-29T11:29:18Z

@andrerom It would awesome if things could be explained in detail and to stick with it. I recognize that eZ kernel does not need to follow SemVer or Symfony's BC promise, but the rules we do decide to follow should be explained in the similar manner to http://symfony.com/doc/current/contributing/code/bc.html (with yes/no tables and everything), so your blog post and proper BC policy docs are much needed.

Symfony BC promise and SemVer aside, what I would definitely like to see kept is eZ's strong BC policy. Deprecate things if you need them being deprecated, but don't flat out remove features users depend on without a proper deprecation period, especially since eZ always prided itself in its BC policy, which even held through with eZ 5 and eZ legacy compatibility.

lserwatka · 2018-01-30T08:19:42Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/content_fields.html.twig

@@ -357,55 +357,59 @@

    {% if hasContent and showMap %}
    {% set apiKey = ezpublish.configResolver.hasParameter('api_keys.google_maps') ? ezpublish.configResolver.getParameter('api_keys.google_maps'): "" %}


This should be removed from here as well. Can you please add info that this config is deprecated now?

alongosz · 2018-01-30T09:55:41Z

Guys, I don't really see a BC problem here :)
Here's why: the purpose and function of this Twig template block is to provide an interactive map for ezgmaplocation field. When updating from current master to this PR, a map is still displayed for content with that field and we even got rid of some nasty browser console errors.
I understand that the map looks different, but still the feature is provided.

Personal POV: I also don't like treating Twig templates as an API and also don't like relying on templates coming from Kernel. These are lightweight minimal examples that should be overridden for the purpose of an actual site. But I know I'm alone in that opinion :)

What I disagree about is deprecating api_keys.google_maps. Using Google Maps or Open Street Map should be a choice for the Developer. The fact that we dropped it in our templates doesn't mean that the developer is not allowed to use Google Maps in his own templates (and deprecating means future removal of the feature). // cc @lserwatka @andrerom

So I'm with @lserwatka previous comment, we should make a doc stating that this changed and how to override template with the version using Google Maps. I find it hard to believe someone on production site didn't need to override this particular template (problem doesn't exist in such case), but maybe that's just me and my approach to dev with eZ ;)

alongosz

Note: It would be nice if @sunpietro or @dew326 could do a review of JS 😀

My review remarks:

alongosz · 2018-01-30T09:28:13Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/content_fields.html.twig

@@ -280,7 +282,6 @@
 {% block ezgmaplocation_field %}
 {##
 # This field type block accepts the following parameters:
- # - string mapType the Google map type (ROADMAP, SATELLITE, HYBRID or TERRAIN), default is ROADMAP
 # - boolean showMap whether to show the Google map or not, default is true


Drop "Google" from the sentence please :)
Also there's the reference to Google's JS API at the bottom of this comment block (can't point it here). Some updated link to OpenStreet doc would be good.

alongosz · 2018-01-30T09:36:44Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/content_fields.html.twig

+            (function (win) {
+                var mapView = function (mapId, latitude, longitude) {
+                    var mapConfig = {
+                        dragging: {{ zoom }},


Is this accurate? I can't find OpenStreetMap API doc on that setting.

It's in the doc http://leafletjs.com/reference-1.3.0.html#map-dragging

But I think {{ zoom }} is not the proper value for dragging parameter. Shouldn't this config looks like this?:

... dragging: {{ draggable }}, zoom: {{ zoom }}, ...

crevillo · 2018-01-30T10:01:03Z

@alongosz yes and no. i mean, customers can have projects using ezpublish kernel 7.0, use the default template for showing the google map and modify other templates to interact with this google map. if we change that to open street map, all the code they have written to interact with gmaps will need to be rewritten also.

we can discuss if this is a bc or not, but otoh, i still think that solution proposed by @gggeek is doable and will have more people happy with it.

emodric · 2018-01-30T10:06:11Z

I also don't like treating Twig templates as an API and also don't like relying on templates coming from Kernel.

I find it hard to believe someone on production site didn't need to override this particular template (problem doesn't exist in such case), but maybe that's just me and my approach to dev with eZ ;)

In my opinion, front facing templates which have a direct impact on how the page looks like in frontend should be considered public API. This includes XSLT transformations for rich text tags, as well as default templates from the kernel. I'm sure there is a number of users who definitely rely on built in templates, or even worse, have other code that directly depends on this template and Google Maps.

But as @andrerom hinted, this all needs a proper discussion with the community to determine what the community wants included in the BC policy.

alongosz · 2018-01-30T10:24:54Z

customers can have projects using ezpublish kernel 7.0, use the default template for showing the google map and modify other templates to interact with this google map. if we change that to open street map, all the code they have written to interact with gmaps will need to be rewritten also.

@crevillo I would rather override template with version supporting gmaps. It's just about copying version supporting GMaps to application. Much simpler than rewriting to OpenStreetMap. How to do it - this is what I've suggested for doc about this change.

That said, maybe it's a good idea to provide some config, however I don't see in my head a proper clean solution yet.
The difference is that our Admin UI already uses OpenStreetMap, so overriding config for site would need to happen in Admin UI, which I don't find very nice. It also would make BC layer working only in case of pure ezpublish-kernel usage without ezplatform-admin-ui, thus in most cases pointless config feature, right?

crevillo · 2018-01-30T10:27:16Z

It also would make BC layer working only in case of pure ezpublish-kernel usage without ezplatform-admin-ui, thus in most cases pointless config feature, right?

Well, i know some cases like this. think in all people that uses ezpublish legacy admin, which, if i'm not wrong, still uses gmaps.

sunpietro · 2018-01-30T10:28:50Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/content_fields.html.twig

+                        false
+                    );
+                } else if (win.attachEvent) {
+                    win.attachEvent(


I think this one can be dropped as we don't support IE10 and older browsers. Please, stick to addEventListener method only.

sunpietro · 2018-01-30T10:29:32Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/content_fields.html.twig

+                    myScript.crossorigin = "";
+                    myCss.rel = "stylesheet";
+                    myCss.href = "https://unpkg.com/leaflet@1.3.1/dist/leaflet.css";
+                    myCss.crossorigin = "";


I'm not sure it's needed.

sunpietro · 2018-01-30T10:29:35Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/content_fields.html.twig

-                    myScript.src = '//maps.googleapis.com/maps/api/js{% if apiKey %}?key={{ apiKey }}{% endif %}';
+                    var myCss = document.createElement('link');
+                    myScript.src = 'https://unpkg.com/leaflet@1.3.1/dist/leaflet.js';
+                    myScript.crossorigin = "";


I'm not sure it's needed.

dew326 · 2018-01-30T10:33:42Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/content_fields.html.twig

+                        dragging: {{ zoom }},
+                        scrollWheelZoom: {{ scrollWheel }}
+                    };
+                    var map = L.map(mapId, mapConfig).setView([latitude, longitude], {{ zoom }});


You don't have to call method .setView() it can be set in config:

... zoom: {{ zoom }}, center: [latitude, longitude], ...

and coords can be cached in variable because are used second time in L.marker()

mikadamczyk added Ready for review Ready for QA labels Jan 26, 2018

mikadamczyk self-assigned this Jan 26, 2018

lserwatka requested review from alongosz, adamwojs, andrerom, webhdx and ViniTou January 26, 2018 15:31

lserwatka changed the base branch from 7.0 to master January 27, 2018 08:53

andrerom reviewed Jan 29, 2018

View reviewed changes

lserwatka requested changes Jan 30, 2018

View reviewed changes

alongosz requested changes Jan 30, 2018

View reviewed changes

sunpietro suggested changes Jan 30, 2018

View reviewed changes

dew326 reviewed Jan 30, 2018

View reviewed changes

mikadamczyk force-pushed the ezp-28647-difference-in-edit-and-preview-mode-for-map-location branch from 013ca2d to 9d4273e Compare January 30, 2018 13:27

EZP-28768: Difference in edit and preview mode for map location

0e6eab9

mikadamczyk force-pushed the ezp-28647-difference-in-edit-and-preview-mode-for-map-location branch from 9d4273e to 0e6eab9 Compare January 30, 2018 14:33

alongosz approved these changes Feb 21, 2018

View reviewed changes

barbaragr self-assigned this Feb 21, 2018

barbaragr changed the title ~~EZP-28768: Difference in edit and preview mode for map location~~ EZP-28647: Difference in edit and preview mode for map location Feb 21, 2018

andrerom approved these changes Feb 21, 2018

View reviewed changes

barbaragr added QA approved and removed Ready for QA labels Feb 21, 2018

barbaragr approved these changes Feb 21, 2018

View reviewed changes

alongosz merged commit 1fb1b70 into ezsystems:master Feb 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-28647: Difference in edit and preview mode for map location #2236

EZP-28647: Difference in edit and preview mode for map location #2236

mikadamczyk commented Jan 26, 2018

gggeek commented Jan 26, 2018

crevillo commented Jan 26, 2018

lserwatka commented Jan 27, 2018

crevillo commented Jan 27, 2018

lserwatka commented Jan 27, 2018

crevillo commented Jan 27, 2018

emodric commented Jan 27, 2018

lserwatka commented Jan 27, 2018

lserwatka commented Jan 27, 2018

emodric commented Jan 27, 2018

lserwatka commented Jan 27, 2018

emodric commented Jan 27, 2018

crevillo commented Jan 27, 2018

crevillo commented Jan 27, 2018

lserwatka commented Jan 27, 2018 •

edited

andrerom commented Jan 29, 2018 •

edited

andrerom Jan 29, 2018

emodric commented Jan 29, 2018

lserwatka Jan 30, 2018 •

edited

alongosz commented Jan 30, 2018

alongosz left a comment

alongosz Jan 30, 2018

alongosz Jan 30, 2018

dew326 Jan 30, 2018 •

edited

crevillo commented Jan 30, 2018

emodric commented Jan 30, 2018 •

edited

alongosz commented Jan 30, 2018

crevillo commented Jan 30, 2018

sunpietro Jan 30, 2018

sunpietro Jan 30, 2018

sunpietro Jan 30, 2018

dew326 Jan 30, 2018 •

edited

		@@ -357,55 +357,59 @@

		{% if hasContent and showMap %}
		{% set apiKey = ezpublish.configResolver.hasParameter('api_keys.google_maps') ? ezpublish.configResolver.getParameter('api_keys.google_maps'): "" %}

EZP-28647: Difference in edit and preview mode for map location #2236

EZP-28647: Difference in edit and preview mode for map location #2236

Conversation

mikadamczyk commented Jan 26, 2018

Description

gggeek commented Jan 26, 2018

crevillo commented Jan 26, 2018

lserwatka commented Jan 27, 2018

crevillo commented Jan 27, 2018

lserwatka commented Jan 27, 2018

crevillo commented Jan 27, 2018

emodric commented Jan 27, 2018

lserwatka commented Jan 27, 2018

lserwatka commented Jan 27, 2018

emodric commented Jan 27, 2018

lserwatka commented Jan 27, 2018

emodric commented Jan 27, 2018

crevillo commented Jan 27, 2018

crevillo commented Jan 27, 2018

lserwatka commented Jan 27, 2018 • edited

andrerom commented Jan 29, 2018 • edited

andrerom Jan 29, 2018

Choose a reason for hiding this comment

emodric commented Jan 29, 2018

lserwatka Jan 30, 2018 • edited

Choose a reason for hiding this comment

alongosz commented Jan 30, 2018

alongosz left a comment

Choose a reason for hiding this comment

alongosz Jan 30, 2018

Choose a reason for hiding this comment

alongosz Jan 30, 2018

Choose a reason for hiding this comment

dew326 Jan 30, 2018 • edited

Choose a reason for hiding this comment

crevillo commented Jan 30, 2018

emodric commented Jan 30, 2018 • edited

alongosz commented Jan 30, 2018

crevillo commented Jan 30, 2018

sunpietro Jan 30, 2018

Choose a reason for hiding this comment

sunpietro Jan 30, 2018

Choose a reason for hiding this comment

sunpietro Jan 30, 2018

Choose a reason for hiding this comment

dew326 Jan 30, 2018 • edited

Choose a reason for hiding this comment

lserwatka commented Jan 27, 2018 •

edited

andrerom commented Jan 29, 2018 •

edited

lserwatka Jan 30, 2018 •

edited

dew326 Jan 30, 2018 •

edited

emodric commented Jan 30, 2018 •

edited

dew326 Jan 30, 2018 •

edited