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

feat: add hover interaction pointer cursor #1342

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

Conversation

silvester-pari
Copy link
Collaborator

@silvester-pari silvester-pari commented Oct 29, 2024

Implemented changes

This PR adds a pointer cursor in situations where we have a "click" select interaction. It uses the Hover interaction from ol-ext in order to achieve this.
Note: for "pointermove" selection, the pointer does not change!
Via options.hover it is also possible to pass further parameters to the interaction (e.g. different cursor, cursor depending on shape, custom callbacks etc.)

Screenshots/Videos

Screencast.from.2024-10-29.11.29.18.webm

Checklist before requesting a review

Copy link

netlify bot commented Oct 29, 2024

Deploy Preview for eoxelements ready!

Name Link
🔨 Latest commit e6862cd
🔍 Latest deploy log https://app.netlify.com/sites/eoxelements/deploys/6721f475b8bce400085cd8b3
😎 Deploy Preview https://deploy-preview-1342--eoxelements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@silvester-pari silvester-pari marked this pull request as ready for review October 29, 2024 11:01
@RobertOrthofer
Copy link
Contributor

I am mildly surprised that we never had this feature...

As this is a true ol-Interaction that is added to the map, since it is not technically part of the interactions-array of the layer, does this get removed somewhere when removing the layer?

@silvester-pari
Copy link
Collaborator Author

silvester-pari commented Oct 29, 2024

As this is a true ol-Interaction that is added to the map, since it is not technically part of the interactions-array of the layer, does this get removed somewhere when removing the layer?

Good point. I added it here, does that make sense?

Copy link
Member

@santilland santilland 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 the implementation! discussion and clarification of the points online, from my side good to merge then

elements/map/package.json Show resolved Hide resolved
@@ -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.

@RobertOrthofer
Copy link
Contributor

As this is a true ol-Interaction that is added to the map, since it is not technically part of the interactions-array of the layer, does this get removed somewhere when removing the layer?

Good point. I added it here, does that make sense?

this should be the correct place, i have tested it and it does get removed when removing the layer, however it sticks around when removing the interaction... I'll have a quick look into this

@RobertOrthofer
Copy link
Contributor

There was indeed a bug with removing the select interaction when updating the layer, i have pushed a fix and updated the test case...

And there was still an .only on the select-interaction tests from the refactoring, causing test cases to be disabled... I hope I didn't break the scope of this PR

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