Skip to content

Conversation

@mwpenny
Copy link
Contributor

@mwpenny mwpenny commented Jul 20, 2016

What is this PR for?

Added a 2D Esri map as a graph option for paragraphs. This makes it easier to visualize relations in geographical data.
Pins are plotted at points in specified longitude/latitude columns and customizable column values are shown when pins are clicked. Is this of interest?

The angular-esri-map module is used, which is released under the Apache 2.0 license, and the JavaScript API is free to use (see https://developers.arcgis.com/terms/).

What type of PR is it?

Feature

What is the Jira issue?

ZEPPELIN-1220

How should this be tested?

Execute a query in a paragraph and select the map pin from the chart type selector.
Set the longitude/latitude (and optionally, pin info) columns, then interact with the map.

Screenshot

screenshot

Questions:

  • Does the licenses files need update? yes. For angular-esri-map.
  • Is there breaking changes for older versions? no
  • Does this needs documentation? potentially. This feature follows the same layout as the other visualization modes, so users will likely be able to use it easily. However, screenshots of some supported visualizations modes are present in the current documentation. If this PR is merged, it may be desirable to display the map option as well. Interestingly the scatter chart is not included in screenshots in the documentation.

@mwpenny mwpenny changed the title Esri [ZEPPELIN-1220] Add geographical map as visualization option Jul 20, 2016
@corneadoug
Copy link
Contributor

@mwpenny Thanks for the PR
There was already some attempts to include Leaflet before, however I think it is not yet to be finished. (No pivot to choose columns at least)
I think the community should be able to decide which map they prefer.

However, in term of license, I'm still not sure if this could be included or not.
Can you provide more infos on how the Javascript API is needed?

@mwpenny
Copy link
Contributor Author

mwpenny commented Jul 21, 2016

@corneadoug, The Esri angular module is used to download and lazy load the JS API and require its various classes. These classes are then used to display the dynamically downloaded map tiles and render the different map layers (i.e., basemap + pin layer).

The angular module was developed by Esri and released under the Apache 2.0 license. They state on their website that "The JavaScript API [downloaded by the angular module] is hosted by Esri and is available for free use".

Esri also has a public Apache 2.0-licensed repo of samples (Esri/quickstart-map-js) which use the API.

@ErikApption
Copy link

@corneadoug This is a great feature! ESRI is by far the most complete mapping component and the base maps are totally free (unlike gg or bing), plus their API is open source. What would be the next step to get this PR accepted?

@mwpenny
Copy link
Contributor Author

mwpenny commented Jul 22, 2016

I've added basemap selection (see screenshot)
zeppelin09

@corneadoug
Copy link
Contributor

@mwpenny Sorry for the delay, I got a bit busy.
I will take a look this week-end. What you can do until then is to rebase with master since there is some conflicts right now.

@mwpenny
Copy link
Contributor Author

mwpenny commented Jul 29, 2016

@corneadoug Done

@corneadoug
Copy link
Contributor

@mwpenny

Alright, so after taking a look, here are my feedbacks:

Overall PR

  • License of angular-esri-map will need to be added in this file (That folder also contains single files if license needs to be copied)
  • There is 2 files to commit: index.html and karma.conf.js which have changes after we build.
  • I think it could be nice to have a page in the documentation, but let's make that in a different PR.

The Map

  • The mouse scroll to zoom in the map should be deactivated, if you scroll down the notebook you might zoom out and then the map vanishes.
  • Could we have the map unzoomed to show the world when loaded (Its fine to keep it centered on Canada ;) )
  • The Map height do not scale when we make the paragraph bigger
  • The Map is going out of the paragraph instead of being contained or fitting the size:
    screen shot 2016-08-02 at 5 03 20 pm
  • When I click on a Pin it moves me somewhere else
    mappin

Options & Pivot

  • Let's remove the height:36px style for the pivot area, I know it was copy pasted from other place in the code, but its better to have all of them with the same height.
  • The BaseMap option isn't removed when we switch graphs, seems to be a mistake here with ng-if inside a class

I will spend more time on checking the code, and would leave inline comments if there is

@mwpenny
Copy link
Contributor Author

mwpenny commented Aug 3, 2016

I have made the changes/fixes you mentioned and rebased with master. I can create the PR for the documentation after this is merged.

@corneadoug
Copy link
Contributor

@mwpenny I tested it again, its so much better now.

You will need to rebase your branch (conflicts, always conflicts)

We could probably improve the marker's popup by adding a bit of padding so that its not too white?
screen shot 2016-08-10 at 2 21 49 pm

Also depending on the paragraph height and the number of infos to show, it seems the popup header is hidden

screen shot 2016-08-10 at 2 24 36 pm

<!-- endbuild -->
<link rel="stylesheet" ng-href="assets/styles/looknfeel/{{looknfeel}}.css" />
<link rel="stylesheet" href="assets/styles/printMode.css" />
<link rel="stylesheet" href="//js.arcgis.com/4.0/esri/css/main.css" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way to have this CSS file locally? It seems it is not inside of angular-map-esri and the map is not rendering well without it.

If the license allows it, it would better to include it in the code assets/styles/ since some people do not have access to internet when using Apache Zeppelin

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it doesn't matter, we can't use ESRI maps without internet

@mwpenny
Copy link
Contributor Author

mwpenny commented Aug 11, 2016

@corneadoug I'm all too familiar with that magic when trying to reproduce bugs ;)

I've made the changes.

  • Small popup: The side and bottom margins of the popup are present due to yesterday's changes (differentiating the white of the popup from the white of the paragraph) and the extra-large top margin is from Esri's CSS. After decreasing the top margin the popup is still very cluttered but a bit more usable.
    zeppelin13
    Even with some extra CSS styling, I just don't think there's enough space to display the popup nicely in a paragraph that small. Other graph modes have a similar problem which users are likely already accustomed to:
    zeppelin14
  • Internet error message: I went with a look similar to the other graph modes:
    zeppelin12
  • Lazy loading: Similar to the way esriLoader loads the map JavaScript, the CSS is now lazy loaded the first time it is needed.

@corneadoug
Copy link
Contributor

@mwpenny With the way we include the css, there is still a risk of not being able to get the css, but having the JS file just fine.
Which end up in the map not showing, and no error message like the one we added in case there is no internet connection. I actually got that a few times with slow internet.

Except for that, LGTM, awesome work!
if anybody wants to try it, I use the test case of #765

@mwpenny
Copy link
Contributor Author

mwpenny commented Aug 12, 2016

@corneadoug The CSS and JavaScript come from the same server. Is the CSS timing out for you but not the JS? Or is the CSS just loading late?

And good to hear!

@corneadoug
Copy link
Contributor

@mwpenny CSS loading late I guess, but there is also a possibility that the resource is not available for whatever reason.

It is not critical for now, but since there is a risk of map not showing for some users without any error message (creating more traffic in the mailing list), it would be nice to try to address it in a different PR maybe.

Merging if there is no more discussions

@asfgit asfgit closed this in 47b1931 Aug 17, 2016
@karuppayya
Copy link
Contributor

@mwpenny Thanks for the awesome work.
I was trying out the visualizations.
I can see only the pins, the maps seems to not load.
Any idea what could be wrong?

@mwpenny
Copy link
Contributor Author

mwpenny commented Aug 17, 2016

@karup1990 Can you send a screenshot of the map? If you look at the network requests in your browser's developer tools, are the requests to the ArcGIS servers successful (i.e., retrieval of the map JavaScript and CSS)? Here's a screenshot of what happens when I load the map (I'm using Firefox):
resload

Also check to make sure the map tile requests are succeeding. Screenshot from my browser:
tileload

@karuppayya
Copy link
Contributor

screen shot 2016-08-17 at 9 13 00 pm
screen shot 2016-08-17 at 9 13 08 pm

Attaching screen capture.
Request seems fine, not sure what is preventing the browser from loading them

@mwpenny
Copy link
Contributor Author

mwpenny commented Aug 17, 2016

@karup1990 Which browser are you using and what is its version? That looks like Chrome.

@karuppayya
Copy link
Contributor

I am using Google Chrome Version 52.0.2743.116 (64-bit) @mwpenny

@karuppayya
Copy link
Contributor

BTw, I tried on Safari just now. And works great :)

@mwpenny
Copy link
Contributor Author

mwpenny commented Aug 17, 2016

@karup1990 I have installed the same version of Chrome and am unable to reproduce the issue. Does clearing your browser cache and cookies help?

@karuppayya
Copy link
Contributor

I cleared the browser cache and cookies. Still see only the pins @mwpenny

@mwpenny
Copy link
Contributor Author

mwpenny commented Aug 17, 2016

@karup1990 This is very strange. I cannot reproduce the issue using the same browser version and your network doesn't seem to be the problem since it is working for you in Safari.

The only other thing I can think of is a browser setting or extension interfering somehow. Try running Chrome with the --disable-extensions argument. If that doesn't work, try creating a new Chrome user profile and using that temporarily.

@corneadoug
Copy link
Contributor

Could be related to the CSS maybe. Any chance it need the css before rendering the map?
@karup1990 no errors in the console?

@mwpenny
Copy link
Contributor Author

mwpenny commented Aug 18, 2016

@corneadoug When I test it without the CSS, the tiles still load but they're not positioned properly (they extend outside of the paragraph).

@karuppayya
Copy link
Contributor

@corneadoug There are no errors on console.
@mwpenny I disabled all extension, and tried view the page in an incognito window. Didnt help

@mwpenny
Copy link
Contributor Author

mwpenny commented Aug 19, 2016

@karup1990 Are you able to test with a fresh Chrome installation (e.g,. using another computer on the same network)?

@tabraiz12
Copy link

adding the following css fixed the issue for safari and chrome

.esri-bitmap {
   max-width: none;
}

@karuppayya
Copy link
Contributor

Works fine for me after the change. Thanks @tabraiz12

@Leemoonsoo
Copy link
Member

Leemoonsoo commented Oct 9, 2016

Dear @mwpenny and Zeppelin PMC.

I have reviewed the license of ArcGIS Online https://developers.arcgis.com/terms/ and and http://www.esri.com/legal/software-license.
Although it's client javascript library is Apache 2, client does not functioning without ArcGIS online service, so the ArcGIS Online license does matter. However, the license does not look like type of license we can use. For example

Generally, if you are not planning to monetize your app, say by charging a fee or by embedding revenue generating advertising in your app, you can make your app available to just about anyone.

and

If you want to charge for access to apps you build using ArcGIS Online you have to have a paid subscription.

Apache 2 License does not have restriction about monetization of Apache Zeppelin. However ArcGIS Online license has incompatible restriction.

Also the client is loading arbitrary code (javascript) dynamically from some remote server on runtime and we can not guarantee what kind of code will be executed on our user's machine.
see https://github.com/apache/zeppelin/blob/master/zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js#L1392

Therefore, unfortunately, we might need to remove this changes from our source tree.

@Leemoonsoo
Copy link
Member

Visualizations have incompatible license can be packaged as a Helium application and loaded dynamically. Soon, there will be highlevel api to make visualization easier based on helium.
In the future, we'll have online registry for applications. So, please consider use helium framework.

asfgit pushed a commit that referenced this pull request Oct 17, 2016
… incompatible license

### What is this PR for?
According to https://developers.arcgis.com/terms/ and and http://www.esri.com/legal/software-license, current map visualization depends on an online service that has incompatible restrictions to Apache 2 license. Please see #1210 (comment)

Possible alternative way is explained [here](#1210 (comment)).

I'm very sad to remove this very good contribution. So please anyone review the license and comment here if you have other opinions, or other way around.

### What type of PR is it?
[Task]

### Todos
* [x] - remove implementation from source tree

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1532

### Questions:
* Does the licenses files need update? yes
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Lee moon soo <[email protected]>

Closes #1501 from Leemoonsoo/ZEPPELIN-1532 and squashes the following commits:

b5ad91a [Lee moon soo] Remove map visualization
dad81c4 [Lee moon soo] Remove map visualization from source tree
darionyaphet pushed a commit to darionyaphet/zeppelin that referenced this pull request Oct 27, 2016
… incompatible license

### What is this PR for?
According to https://developers.arcgis.com/terms/ and and http://www.esri.com/legal/software-license, current map visualization depends on an online service that has incompatible restrictions to Apache 2 license. Please see apache#1210 (comment)

Possible alternative way is explained [here](apache#1210 (comment)).

I'm very sad to remove this very good contribution. So please anyone review the license and comment here if you have other opinions, or other way around.

### What type of PR is it?
[Task]

### Todos
* [x] - remove implementation from source tree

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1532

### Questions:
* Does the licenses files need update? yes
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Lee moon soo <[email protected]>

Closes apache#1501 from Leemoonsoo/ZEPPELIN-1532 and squashes the following commits:

b5ad91a [Lee moon soo] Remove map visualization
dad81c4 [Lee moon soo] Remove map visualization from source tree
pedrozatta pushed a commit to pedrozatta/zeppelin that referenced this pull request Oct 27, 2016
… incompatible license

### What is this PR for?
According to https://developers.arcgis.com/terms/ and and http://www.esri.com/legal/software-license, current map visualization depends on an online service that has incompatible restrictions to Apache 2 license. Please see apache#1210 (comment)

Possible alternative way is explained [here](apache#1210 (comment)).

I'm very sad to remove this very good contribution. So please anyone review the license and comment here if you have other opinions, or other way around.

### What type of PR is it?
[Task]

### Todos
* [x] - remove implementation from source tree

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1532

### Questions:
* Does the licenses files need update? yes
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Lee moon soo <[email protected]>

Closes apache#1501 from Leemoonsoo/ZEPPELIN-1532 and squashes the following commits:

b5ad91a [Lee moon soo] Remove map visualization
dad81c4 [Lee moon soo] Remove map visualization from source tree
@volumeint
Copy link
Contributor

I just published a helium visualization that uses Leaflet. You can find it at (https://github.com/volumeint/helium-volume-leaflet). To get it working, I first had to patch webpack.config.js in zengine 0.8.0-SNAPSHOT to add a test for png files. Leaflet has two png files referenced in their CSS.

    test: /\.svg(\?\S*)?$/, loader: 'url-loader', }, {
    test: /\.png(\?\S*)?$/, loader: 'url-loader', }, {       <-- add this line
    test: /\.json$/, loader: 'json-loader' }, ],

@Leemoonsoo
Copy link
Member

@volumeint cool! Do you mind make a pullrequest with modification test: /\.png(\?\S*)?$/, loader: 'url-loader' ?

@volumeint
Copy link
Contributor

Just closing the loop. Pull Request #2313 contained the webpack.config.js change. It has been merged into master.

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.

7 participants