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

feat: add hover interaction pointer cursor #1342

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: add hover interaction pointer cursor #1342

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
023908b
feat: add hover interaction pointer cursor
silvester-pari Oct 29, 2024
9fcc629
chore: add test to check if the `hover` options are passed
silvester-pari Oct 29, 2024
ad15ea7
fix: remove hover interaction when removing select interaction
silvester-pari Oct 29, 2024
cd6a40d
chore: formatting
silvester-pari Oct 29, 2024
cc6d2dd
properly remove interaction on update
RobertOrthofer Oct 30, 2024
d9e42ad
enable all select tests
RobertOrthofer Oct 30, 2024
e6862cd
test select interaction removal via updating the layer
RobertOrthofer Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions elements/map/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@
"flatgeobuf": "^3.35.0",
"lit": "^3.0.2",
"ol": "^10.2.0",
"ol-ext": "^4.0.24",
Copy link
Member

Choose a reason for hiding this comment

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

is this the first time we use ol-ext? do we need this really to solve the problem, i'm in principle ok with it, just trying to understand if this might have other implications like not being able to update OL because ol-ext does not support the new version yet, or something like that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this would be the first time using it, but probably not the last time. You're right, it could cause troubles when updating 🤔
An alternative would be implementing this functionality ourselves, where I would hope for support from @RobertOrthofer

Copy link
Contributor

Choose a reason for hiding this comment

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

It does look like a bit of an overkill for this specific feature, as implementation would be quite trivial, something along the way of this:

  map.on('pointermove', (e) => {
    if (e.dragging) return;
    map.getTargetElement().style.cursor = map.hasFeatureAtPixel(pixel, {layerfilter, ...options}) ? 'pointer' : ''
  }

but I'm not strictly against using ol-ext, they offer some useful or fancy stuff, and implementations are quite clean for the most part.

... there might be an argument for not having this as an interaction that is added with a select layer, but rather have the pointer-changer as something the map simply does. The information if the pointer should change should be available on each layer, so if there is a feature at the pixel, and the layer of the feature has an active select interaction, the cursor could be changed to pointer.

The pro of this is the possibility of multiple hover-interactions without interfering, the con is that I don't see an easy way of doing this with a hitTolerance

Copy link
Member

Choose a reason for hiding this comment

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

Difficult decision, am i understanding that in principle we are fine with this? So we go for it, and worst case if it creates trouble down the line we re-evaluate if we keep ol-ext? I would mark this as resolved then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to have another option, I have created an alternative in #1348; it uses the pointermove event as @RobertOrthofer suggested, which makes the entire thing much simpler. The only issue I am currently facing is that I didn't find a way to test this.

"ol-stac": "^1.0.0-beta.10",
"proj4": "^2.9.2"
},
"devDependencies": {
"@types/ol-ext": "github:siedlerchr/types-ol-ext#a76d90780322f405e6fd4ac1756cb7de89ac387b",
santilland marked this conversation as resolved.
Show resolved Hide resolved
"@types/proj4": "^2.5.4",
"vite": "^5.0.2"
},
Expand Down
20 changes: 20 additions & 0 deletions elements/map/src/helpers/select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import Feature from "ol/Feature";
import RenderFeature from "ol/render/Feature";
import VectorLayer from "ol/layer/Vector";
import { createEmpty, extend, isEmpty } from "ol/extent";
import Hover from "ol-ext/interaction/Hover";

/**
* @typedef {import('../main').EOxMap} EOxMap
Expand Down Expand Up @@ -215,6 +216,23 @@ export class EOxSelectInteraction {
}
};
eoxMap.map.getLayerGroup().on("change", changeLayerListener);

/**
* Sets up the ol-ext Hover interaction
* for the selection layer
* and passes the `options.hover` object
*/
this.hover = new Hover({
...options.hover,
layers: [this.selectLayer],
});
this.hover.setActive(this.active);
this.eoxMap.map.addInteraction(this.hover);
if (options.condition === "click") {
this.hover.on("enter", () => {
this.hover.setCursor("pointer");
});
}
}

/**
Expand All @@ -223,6 +241,7 @@ export class EOxSelectInteraction {
*/
setActive(active) {
this.active = active;
this.hover?.setActive(active);
}

/**
Expand Down Expand Up @@ -281,6 +300,7 @@ export class EOxSelectInteraction {
remove() {
this.selectStyleLayer.setMap(null);
delete this.eoxMap.selectInteractions[this.options.id];
this.eoxMap.map.removeInteraction(this.hover);
this.selectLayer.un("change:source", this.changeSourceListener);
}

Expand Down
10 changes: 10 additions & 0 deletions elements/map/test/cases/hover/add-select-interaction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,16 @@ const addSelectInteraction = () => {
// moving the cursor to a feature, moving it off the feature, and onto the feature again
expect(featureSelectCounter).to.be.equal(2);
}
expect(
eoxMap.map
.getInteractions()
.getArray()
.find(
(i) =>
i.cursor_ ===
vectorLayerStyleJson[0].interactions[0].options.hover.cursor,
),
).to.exist;
});

eoxMap.map.on("loadend", () => {
Expand Down
3 changes: 3 additions & 0 deletions elements/map/test/fixtures/hoverInteraction.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
"stroke-color": "red",
"stroke-width": 3
}
},
"hover": {
"cursor": "zoom-in"
}
}
}
Expand Down
1 change: 1 addition & 0 deletions elements/map/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export type SelectOptions = Omit<
modify?: boolean;
type?: string;
geometryFunction?: import("ol/interaction/Draw").GeometryFunction;
hover?: import("ol-ext/interaction/Hover").Options;
};

export type ControlOptions = import("ol/control/Control").Options;
Expand Down
Loading