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

Map/feature/interaction props #400

Merged
merged 16 commits into from
Nov 27, 2023
Merged

Conversation

RobertOrthofer
Copy link
Collaborator

This PR changes the way interactions (draw and select) are added to the EOxMap.
Interactions are now part of the EOxLayer-Definition, just like the source or the style. This represents most real-world applications, as interactions usually are linked to a specific layer, especially the implemented draw and select (including hover)-interactions. The interactions are generated as part of the layer generation, thus guaranteeing that the layer already exists.
This also means that the addDraw and addSelect-Methods became obsolete, and so is the hassle of having to wait for the initial render until interactions can be set. Therefore the play-functions in the storybook are not needed anymore.

I did not succeed in making the layers-attribute interactive and forcing re-renders of the component with the new arguments, but minor documentation improvements as well as a fix for the ab-compare have been added as part of this PR.

Copy link
Collaborator

@silvester-pari silvester-pari left a comment

Choose a reason for hiding this comment

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

Thanks for this, looks really promising. One issue I encountered is that other elements (such as eox-drawtools and eox-itemfilter actually use the addDraw method, in the case of the drawtwools for example drawing is not initially enabled, but only added once the user clicks the "draw" button.

This probably can be mitigated by initializing the interaction with active: false and then only do setActive(true) when clicking the button, but I didn't manage to get this working.

Not too sure how to procede, but in any case it currently breaks the buiding and testing pipelines.

Edit: I was successful in some way, but it seems that Draw doesn't respect the active property active when initializing. So something like

new Draw({
  active: false
})

does nothing, so I tried changing draw.ts to include

if (options_.active === false) {
  drawInteraction.setActive(false);
}

which allows to initialize it like this:

      interactions: [
        {
          type: "draw",
          options: {
            active: false,
            id: "drawInteraction",
            type: this.type,
            modify: this.allowModify,
            stopClick: true,
          },
        },
      ],

and then, at a later point in time, only thing needed to toggle the interaction is

this.draw.setActive(true / false);

In general, this means that in tools like the eox-drawtools it brings the layer configuration and the draw configuration closer together, which I think is a good thing. If someone wants to add a drawing functionality to a map, the person might not be interested in adding an extra empty vector layer and then adding a draw interaction to that, but just adding the drawtools would do both things at the same time. I added this in the map/feature/interaction-props-adaptations branch for now and created #425, but we might need to pick out some parts that are relevant and strip out others in a separate branch.

@StefanBrand
Copy link
Member

In general I like that addSelect and removeSelect do not longer have to be handled by the parent application. Some comments are detailed below.

Current Usage

Currently we're doing something like this for both select and hover interactions:

      const eoxmap = this.getEoxMap();
      if (Object.hasOwn(eoxmap.selectInteractions, "selectInteraction"))
        eoxmap.removeSelect("selectInteraction");
      document.querySelector("eox-map").addSelect(this.selectedLayer.id, {
        id: "selectInteraction",
        condition: "singleclick",
        idProperty: "id",
        style: {
          "stroke-color": "white",
          "stroke-width": 3,
        },
      });

Configuring Interactions

The new verbose form of configuring interactions contains details that should be hidden from the parent application. In particular the type and source of the interaction layer are too much detail:

{
"type": "select",
"options": {
"id": "selectInteraction",
"condition": "pointermove",
"layer": {
"type": "Vector",
"properties": {
"id": "selectLayer"
},
"source": {
"type": "Vector"
},
"style": {
"stroke-color": "red",
"stroke-width": 3
}
}
}
}

I see that already a shorthand configuration has been added to mitigate this:

// a layer can be defined by only its style property as a shorthand.
const originalJsonDefinition = this.selectLayer.get("_jsonDefinition");
layerDefinition = {
...originalJsonDefinition,
style: options.style,
properties: {
id: this.selectLayer.get("id") + "_select",
},
source: {
type: originalJsonDefinition.type,
},
} as EoxLayer;

Removing Interactions

When switching pages and setting new layers with interactions to the map, we must remove the previously added interaction beforehand:

      if (Object.hasOwn(eoxmap.selectInteractions, "selectInteraction"))
        eoxmap.removeSelect("selectInteraction");

I see that removeInteraction is untouched by this PR. I think that it would be consistent if the attached interaction was also removed automatically when a layer is removed from the map.

@silvester-pari silvester-pari merged commit 808539b into main Nov 27, 2023
3 checks passed
@silvester-pari silvester-pari deleted the map/feature/interaction-props branch November 27, 2023 18:02
@silvester-pari silvester-pari mentioned this pull request Nov 27, 2023
2 tasks
@silvester-pari
Copy link
Collaborator

I see that removeInteraction is untouched by this PR. I think that it would be consistent if the attached interaction was also removed automatically when a layer is removed from the map.

This is now tracked in #471.

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.

3 participants