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

Migrate to mapbox #997

Closed
28 tasks done
HarelM opened this issue Mar 28, 2019 · 24 comments
Closed
28 tasks done

Migrate to mapbox #997

HarelM opened this issue Mar 28, 2019 · 24 comments
Assignees
Labels
enhancement High Needed but there's a work around
Milestone

Comments

@HarelM
Copy link
Member

HarelM commented Mar 28, 2019

Infra

As part of the effort to implement #971 we have seen the the peformance of openlayer isn't good enough for detailed map. therefore we need to migrate the code from open layers to mapbox...

  • line animation
  • start and end green-red markers
  • traces
  • recording color
  • handle drag end of marker - click should not happen
  • location marker
  • move hard-coded string to resources
  • clean poi edit interaction class
  • overlay order when select-deselect?
  • default glyph and sprite
  • close popup of cluster after click
  • remove all references to openlayers
  • remove all aol-... html references
  • controls css
  • add arcgis support?
  • add rtl support
  • pointer counter check for touch
  • touch events for route planning
  • fix icons sprite address
  • allow hebrew fonts using https://github.com/orangemug/font-glyphs
  • Gps arrow is missing in mobile app.
  • Rotation should use animation when GPS is on.
  • When finishing recording the route should refresh to match the right color.
  • Hebrew doesn't work well in cordova - probably related to finding the local file.
  • Remove attribution when selecting a vector layer as it occupies the statistics space
  • allow usage of fonts offline? will be addressed as part of offline support feature
  • bring back snappings
  • improve route editing performance ignoring this for now as it's reasonable
@HarelM HarelM self-assigned this Mar 28, 2019
@HarelM HarelM added enhancement High Needed but there's a work around labels Mar 28, 2019
@HarelM HarelM added this to the Next Release milestone Mar 28, 2019
@HarelM
Copy link
Member Author

HarelM commented Apr 1, 2019

@HarelM
Copy link
Member Author

HarelM commented Apr 1, 2019

Also this will probably be needed to support integer zoom levels:
mapbox/mapbox-gl-js#1695

@zstadler
Copy link
Member

zstadler commented Apr 1, 2019

Also this will probably be needed to support integer zoom levels:
mapbox/mapbox-gl-js#1695

IMHO, having non-integer zoom levels is a benefit for the user. On a small screen, integers zoom levels could be either too small or too large. Non-integer zoom levels are also beneficial when trying to fit a route to the screen size.

On the other hand, using integer zoom levels for the on-screen buttons is probably the right thing to do.

@valleyofdawn
Copy link

@HarelM
Copy link
Member Author

HarelM commented Apr 5, 2019

Ant-path animation for mapbox - very similar to how openlayers work:
https://jsfiddle.net/2mws8y3q/

@HarelM
Copy link
Member Author

HarelM commented Apr 7, 2019

Drag a point - needed for editing:
https://docs.mapbox.com/mapbox-gl-js/example/drag-a-point/

@HarelM
Copy link
Member Author

HarelM commented Apr 17, 2019

Click on a layer to get the right feature to do something:
https://docs.mapbox.com/mapbox-gl-js/example/popup-on-click/

@HarelM
Copy link
Member Author

HarelM commented Apr 21, 2019

@valleyofdawn @zstadler FYI: it seems like there can be only one sprite for the map:
See here:
mapbox/mapbox-gl-js#4086
And here:
https://stackoverflow.com/questions/36867301/multiple-icon-sprites-with-mapbox-gl-js
Same goes for the font stack.
I suggest to create a sprite that will serve both IHM and IMTBM as add it to the site.

@zstadler
Copy link
Member

Yes, the existing sprite is intended for both maps and includes the union of the required icons.

@HarelM
Copy link
Member Author

HarelM commented Apr 22, 2019

Houston, we have a problem:
Just ran into this issue:
mapbox/mapbox-gl-js#2171
It mean we won't be able to support external tile server that do not support CORS, which is a big problem...

HarelM added a commit that referenced this issue Apr 22, 2019
…fixed expression changed after check error
HarelM added a commit that referenced this issue Apr 24, 2019
…w hebrew fonts, fix icons sprite address, zoom-in-out round to integer, fix osm progressbar color.
HarelM added a commit that referenced this issue Apr 25, 2019
…on cordoova, fix missing animation on rotation.
HarelM added a commit that referenced this issue Apr 25, 2019
…ase the address is not the site origin, Fix end of recording color refresh.
@valleyofdawn
Copy link

The "record" button is still present in the web version.
The "northing" button rotates the map back to "north up" but doesn't straiten the map pitch.

@zstadler
Copy link
Member

Cannot view the PEF map
http://213.8.25.66/arcgis/rest/services/BaseMaps/PEFMap/MapServer

No GET requests for tiles in this map source.

@HarelM
Copy link
Member Author

HarelM commented Apr 26, 2019

See my comment above related to CORS, it might also be related to the fact that this server is not secured (https). But I'm writing these lines without proper debug so I might be wrong...

@zstadler
Copy link
Member

The "northing" button rotates the map back to "north up" but doesn't straiten the map pitch.

The map pitch can be easily reset with a pitch-down gesture (ctrl-mouse-drag down).
I'm not sure the "northing" button should change the map pitch.

@valleyofdawn
Copy link

valleyofdawn commented Apr 26, 2019

The map pitch can be easily reset with a pitch-down gesture (ctrl-mouse-drag down).
I'm not sure the "northing" button should change the map pitch.

It's a minor issue, but I'm used to this convention from other apps, save a few secs.

@HarelM
Copy link
Member Author

HarelM commented Apr 26, 2019

@zstadler I have added the following code in order to support arcgis:

address += "/export?dpi=96&transparent=true&format=png32&bbox={bbox-epsg-3857}&bboxSR=3857&imageSR=3857&size=256,256&f=image";

Which should export the right piece of the map and present it.
I think the PEF server doesn't fully supports this feature at it returns transparent tiles.
The code above might be incorrect too, but I'm not an ArgGIS expert, if you think the address change is incorrect or should be altered please let me know - this was a trail and error on my behalf to make an ArcGIS example work...

@HarelM
Copy link
Member Author

HarelM commented Apr 26, 2019

Furthermore related to PEF:
I was able to switch to mixed content in my desktop browser and I was able to get the map working.
The following turned from an error to a warning:
Cross-Origin Read Blocking (CORB) blocked cross-origin response http://213.8.25.66/arcgis/rest/services/BaseMaps/PEFMap/MapServer/ with MIME type text/html. See https://www.chromestatus.com/feature/5629709824032768 for more details.
So there's no doubt it is related to the difference openlayers and mapbox fetch images.
I'm not sure how to solve this, I couldn't find anything in the docs to indicate how to workaround this.
Adding https://cors-anywhere.herokuapp.com/ doesn't help too for some reason, it might be that ArcGIS actively blocks this...
image

@HarelM
Copy link
Member Author

HarelM commented Apr 26, 2019

The following address seems to workaround all the issues I've mentioned:
https://cors-anywhere.herokuapp.com/http://213.8.25.66/arcgis/rest/services/BaseMaps/PEFMap/MapServer/tile/{z}/{y}/{x}

@zstadler
Copy link
Member

Great!

I don't know why the cors-anywhere.herokuapp.com prefix didn't work for me when I tried it. Perhaps there was some typing error.

I suggest the client will add a https://cors-anywhere.herokuapp.com/ prefix to every http source in a user-invisible way.

@HarelM
Copy link
Member Author

HarelM commented Apr 27, 2019

That's a great idea! implemented, yet to be released.

@valleyofdawn
Copy link

Overall very robust and bug-free.

  1. Custom maps: http doesn’t work - only https.
  2. Some https custom maps won't show either. Such as: מנדטורית שנות הארבעים, Strava heatmaps and several orthophotos.
  3. The CORSanywhere workaround works just in some of the cases
  4. Image POIs - the text overlaps with the image. (see below)
  5. The legend doesn’t work with the vector map
    image

@HarelM
Copy link
Member Author

HarelM commented Apr 27, 2019

CORS anywhere will fix (some) http maps - I'm afraid there's nothing more I can do.
I've fixed the legend.
I can't reproduce the text image and the text overlap, this is how I see it on my desktop:
Tested both FF and Chrome, do you have special settings on your PC?
image

@HarelM
Copy link
Member Author

HarelM commented Apr 27, 2019

Never mind, when image has no icon the text overlaps. I'll fix it.

HarelM added a commit that referenced this issue Apr 27, 2019
* Migrate to mapbox #997 - Inital and main commit.

* Migrate to mapbox #997 - added limited support to arcgis, added rtl, fixed expression changed after check error

* Migrate to mapbox #997 - remove console.log...

* Migrate to mapbox #997 - Fix weird touch issue after editing is completed.

* Migrate to mapbox #997 - support touch events for route editing, allow hebrew fonts, fix icons sprite address, zoom-in-out round to integer, fix osm progressbar color.

* Migrate to mapbox #997 - attempt to fix missing icon and script file on cordoova, fix missing animation on rotation.

* Migrate to mapbox #997 - Fix lint, Fix issue with click on point to edit.

* Migrate to mapbox #997 - Fix relative path for image load error

* Migrate to mapbox #997 - Fix route palnning (again...), Fix icon in case the address is not the site origin, Fix end of recording color refresh.

* Migrate to mapbox #997 - wrong base address for cordova device.

* Migrate to mapbox #997 - Fix rotation and gps icon for the last time hopefully.

* Migrate to mapbox #997 - bring back info control.

* remove failing test

* Migrate to mapbox #997 - remove attribution, update technology stack.

* Migrate to mapbox #997 - Added https CORS support in case of http address.

* Migrate to mapbox #997 - remove recording button from web.

* Migrate to mapbox #997 - Fix legend.

* Migrate to mapbox #997 - Fix image text overlap - missing icon...

* Migrate to mapbox #997 - bring back snapping
@HarelM
Copy link
Member Author

HarelM commented Apr 27, 2019

Branch was merged to master, all pending issues have been resolved, beta site was updated with latest changes.
Please open separate issues for stuff we find from no on...

@HarelM HarelM closed this as completed Apr 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement High Needed but there's a work around
Projects
None yet
Development

No branches or pull requests

3 participants