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

PR for #38 permalink #121

Merged
merged 10 commits into from
Jul 17, 2020
Merged

PR for #38 permalink #121

merged 10 commits into from
Jul 17, 2020

Conversation

justb4
Copy link
Collaborator

@justb4 justb4 commented Mar 23, 2020

This is a minimal permalink implementation, it is configuration-driven. Using a new permalink config object, example:

  "permalink": {
    "location": "hash",                  # how to specify in address bar hash(#) default or query(?)
    "projection": "EPSG:4326",   # projection for permalink coords (default: map projection)
    "paramPrefix": "",                   # prefix for permalink parameters (default: empty str)
    "history": true,                        # should history be kept and shown in browser history (default: false)
    "precision": 6                          # for numbers: center, zoom, extent
  },

Defaults apply, so "permalink": {} is also valid.

  • Encoding: using c (center), z (zoom) , r (rotation) with optional prefixes e.g. map_c=.
  • Example: http://localhost:8081/#z=1.9867&c=2.306,39.5258&r=0
  • Non-map related params, e.g. appCtx= are preserved
  • optional history popstate capture
  • optional parameter prefixes: map_z=1.9867&map_c=2.306,39.5258&map_r=0
  • optional projection in permalink, e.g. use WGS84 i.s.o. OSM Merc coords
  • Implementation in new class src/components/ol/PermalinkController.js as ordinary class
  • Allow for overriding in Map.vue/PermalinkController.js app-specific subclasses via Factory Method.
  • prepared for URL sharing (PermalinkController.getParamStr ()) method
  • prepared for IFrame Embed sharing (PermalinkController.getEmbedHTML ()) method
  • allow map extent URL parameter i.s.o. center+zoom (configurable)
  • allow layers (lids?) to be encoded (challenge: layers are loaded async)
  • Unit Tests

@justb4
Copy link
Collaborator Author

justb4 commented Mar 25, 2020

All planned permalink features now implemented. Need to revisit layer handling when #113 is merged (async layer creation).

@justb4
Copy link
Collaborator Author

justb4 commented May 8, 2020

So this was already there, only unit tests are lacking. The ShareButton component (#128) builds on this PR.

@justb4 justb4 changed the title WIP for #38 permalink PR for #38 permalink Jul 10, 2020
@justb4
Copy link
Collaborator Author

justb4 commented Jul 10, 2020

Should be good to go.

lConf.lid = now.getTime();
// Make a unique layerId from Layer name and URL so contexts
// like permalinks can be reapplied.
lConf.lid = btoa(lConf.url + lConf.name).substr(0, 6);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does every layer has a property url? Not sure about that

@chrismayer
Copy link
Collaborator

chrismayer commented Jul 10, 2020

Looks good to me, thanks @justb4. One little question above and one general question:

Will it be easily possible to apply another permalink structure? I have a public application called Shareloc, which provides shareable links for OpenLayers maps. In the future I might want to upgrade Shareloc to Wegue. The permalink in this PR looks a bit different from the one used with Shareloc (e.g. https://apps.meggsimum.de/shareloc/share.html?zoom=7&X=8.541570112005266&Y=50.5918788090799&bgLayer=osm.base). So I would need to map / modify the parameter keys and maybe add more. Would this easily possible with the approach in this PR? TIA!

@justb4
Copy link
Collaborator Author

justb4 commented Jul 13, 2020

Not directly, though there is some abstraction already via the config: paramPrefix which is reflected in the permalink param names and their encoding/decoding. This was somewhat similar as in GeoExt.

A next step would be to make all param-names configurable with sensible default. Not too difficult as parameter naming is just in a few places (2). Only bgLayer is a bit specific in the Shareloc approach. In the current approach there is just layers as some Layers are already background-type (isBaseLayer).

@chrismayer
Copy link
Collaborator

Not directly, though there is some abstraction already via the config: paramPrefix which is reflected in the permalink param names and their encoding/decoding. This was somewhat similar as in GeoExt.

A next step would be to make all param-names configurable with sensible default. Not too difficult as parameter naming is just in a few places (2). Only bgLayer is a bit specific in the Shareloc approach. In the current approach there is just layers as some Layers are already background-type (isBaseLayer).

Thanks for clarification.

@chrismayer
Copy link
Collaborator

I just tested the Permlink behaviour and found something unexpected. If a layer gets activated it is not added to the permalink. Steps to reporduce:

grafik

@justb4
Copy link
Collaborator Author

justb4 commented Jul 14, 2020

Confirmed. As soon as you alter the Map View (pan, zoom) the Layer is added. Almost sure the problem is that changing active Layers does not trigger moveend from the OL Map (to which PermalinkController listens). Investigating. May need to listen to other generic Event, change?

@chrismayer
Copy link
Collaborator

It is not intended that activating a layer triggers moveend on the map. I think listening to change:visible on the map's LayerGroup should be the correct event. Derivable by map.getLayerGroup().

@justb4
Copy link
Collaborator Author

justb4 commented Jul 14, 2020

Yes, aware by now. Approach now (before seeing comment above) is to listen to two additional events: 'change:visible' for each Layer and 'change:length' for the Map's Layer Collection. The latter is needed whenever Wegue support dynamic Layer addition. Also added tests and this approach works. Tried listening to change:visible for Map's LayerGroup but this had no effect. But I was not familiar with LayerGroup.

I will push the changes for now and make it WIP for review.

@chrismayer
Copy link
Collaborator

Thanks for your ongoing work @justb4 ! LGTM, I'll merge now.

@chrismayer chrismayer merged commit f76a6b8 into wegue-oss:master Jul 17, 2020
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