Add Bookmarking Sets of Layers #1185
Conversation
✅ Deploy Preview for labs-zola ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -6,6 +7,25 @@ import { Promise } from 'rsvp'; | |||
| export default Controller.extend({ | |||
| mainMap: service(), | |||
| metrics: service(), | |||
| router: service(), | |||
|
|
|||
| savedLayerSets: window.localStorage['saved-layer-sets'] | |||
There was a problem hiding this comment.
This might be more trouble than it's worth but did you consider making an Ember model for these bookmarkable layer sets, similar to how we have a bookmark model already? This approach isn't bad but using a model might be more Ember-y
There was a problem hiding this comment.
I considered it, but abandoned the idea after looking at the way the bookmarks were set up and finding the abstraction was a little much for me.
There was a problem hiding this comment.
How about you spend another day or two on that approach? We already use LocalStorageSerializer in the serializer for bookmarks and you may be able to take advantage of the existing layer group model for a property on your new model.
Keeping all of this in a controller feels like breaking Ember's conventions which tends to lead to tech debt. I'm more comfortable taking on debt in this repo than I would be in a newer thing, but I think this situation warrants spending a little more time trying to do it the "ideal" way.
There was a problem hiding this comment.
@dhochbaum-dcp Have you spent any more time on the approach I described here? If you have and are still hitting a wall, I won't hold up the PR over it.
There was a problem hiding this comment.
I spent another day on it and got nowhere.
There was a problem hiding this comment.
Ok. I'll take another look at the PR as is and leave a review.
| @@ -6,6 +7,25 @@ import { Promise } from 'rsvp'; | |||
| export default Controller.extend({ | |||
| mainMap: service(), | |||
| metrics: service(), | |||
| router: service(), | |||
|
|
|||
| savedLayerSets: window.localStorage['saved-layer-sets'] | |||
There was a problem hiding this comment.
How about you spend another day or two on that approach? We already use LocalStorageSerializer in the serializer for bookmarks and you may be able to take advantage of the existing layer group model for a property on your new model.
Keeping all of this in a controller feels like breaking Ember's conventions which tends to lead to tech debt. I'm more comfortable taking on debt in this repo than I would be in a newer thing, but I think this situation warrants spending a little more time trying to do it the "ideal" way.
This PR allows users to bookmark the current layers, and access their bookmarked layers in the saved menu.
Completes #1192