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.

Sign up for GitHub

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

Jump to bottom

Normalise OL Layer's naming inside Hajk #883

Closed
jacobwod opened this issue Oct 21, 2021 · 3 comments · Fixed by #1094
Closed

Normalise OL Layer's naming inside Hajk #883

jacobwod opened this issue Oct 21, 2021 · 3 comments · Fixed by #1094
Assignees
@jacobwod @Hallbergs
Labels
module:admin module:client/core Core functionality (not a plugin) optimization plugin:layerswitcher Functionality and features of the LayerSwitcher plugin
Milestone
3.11

Comments

@jacobwod
Copy link
Member

When we add a Layer to the Map, we currently set some additional properties. Two of them are redundant: name, and type - where name is used mostly (and is, in fact a wrong term, as it's more of an id).

In addition, in some cases we add a Layer to the Map, without supplying neither name, type nor caption.

Here's an example for [l.get("name"), l.get("type"), l.get("caption")]:
Skärmavbild 2021-10-21 kl 11 39 18

In the example above, 29-30 should have a name added, 31 should have the type value moved to name (and all references in code must be changed, naturally). Finally, all names should follow the same casing, be it camelCase or kebab-case.
@jacobwod jacobwod added this to the 3.x milestone Oct 21, 2021
@Hallbergs
Copy link
Member

Let's hold this off until we're rewriting the LayerSwitcher-plugin

@Hallbergs Hallbergs added plugin:layerswitcher Functionality and features of the LayerSwitcher plugin module:admin module:client/core Core functionality (not a plugin) labels Nov 23, 2021
jacobwod added a commit that referenced this issue Dec 16, 2021
@jacobwod
Initial fixes for #883:
3db14a5 
- I've added the 'name' property wherever I couldn't see any.
- I called the layers 'coreFoo' or 'pluginFoo' so far, depending on if the layer is added in Core or by a Plugin. I'm not sure this is the best way to go, perhaps we could use another prefix or syntax.
- Those layers that already had a 'type' property were not modified: I only added (duplicated) the existing value from 'type', while keeping it. Replacing 'type' with 'name' will require more search-and-replace, which I'm not doing right now.
jacobwod added a commit that referenced this issue Mar 25, 2022
@jacobwod
Added a marker that shows where the click event occured:
73bd8d5 
- Currently the marker is hard-coded (never a good solution.
- In order to make it a setting we need to differentiate it from the current Infoclick settings. Right now there is no way to set a marker for click event. The marker that exists in Admin is for actually marking the clicked feature (if it is a Point feature). We want to keep this setting too, and it's possible that we want different indicators for those two points (one for the 'clicked here' marker and another one to actually mark a selected feature).
- Also, I tried to give some reasonable properties to the vector layer needed for this. Please see #883 for more info.
@jacobwod
Copy link
Member Author

While implementing #1086 I came into the conclusion that this is very much a pre-requirement for a successful implementation of #1086. Even if we don't fix the naming/caption confusion right away, we should at least add a type property to all layers, with one of the following values: system, layer, groupLayer (perhaps more?).

Without this #1086 gets unnecessarily complicated.

@jacobwod
Copy link
Member Author

I dropped the groupLayer idea as we already had a group value which did the same.

In addition, I dropped the system property, in favour of the existing layerType property. However, it got moved from the ol/layer object itself and into it's "hidden" values_ property. This way, we can use the getter/setter as intended for layers properties in OpenLayers.

Now the system layers look way better: correct type, coherent name and (as a bonus) caption that can be used in the UI:
Skärmavbild 2022-05-24 kl 14 26 22

@jacobwod jacobwod pinned this issue May 25, 2022
@jacobwod jacobwod mentioned this issue May 30, 2022
Merged
@jacobwod jacobwod linked a pull request May 30, 2022 that will close this issue
Feature/883 layer naming #1094
Merged
@jacobwod jacobwod modified the milestones: 3.x, 3.11 Jun 1, 2022
@jacobwod jacobwod unpinned this issue Jun 14, 2022
@jacobwod jacobwod reopened this Jun 23, 2022
@jacobwod jacobwod closed this as completed Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jacobwod jacobwod

@Hallbergs Hallbergs

Labels
module:admin module:client/core Core functionality (not a plugin) optimization plugin:layerswitcher Functionality and features of the LayerSwitcher plugin
Projects
None yet
Milestone
3.11
Development

Successfully merging a pull request may close this issue.

Feature/883 layer naming hajkmap/Hajk
2 participants
@jacobwod @Hallbergs