-
Notifications
You must be signed in to change notification settings - Fork 41
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
infoClick on multiple visible features/layers #367
Conversation
Hi @enricofer, Could you please run Greets Felix |
computed: { | ||
numfeaTts() { | ||
return 23 | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be unused.
@@ -115,29 +130,65 @@ export default { | |||
*/ | |||
onMapClick (evt) { | |||
const me = this; | |||
me.features = [] | |||
const featureLayer = me.map.forEachFeatureAtPixel(evt.pixel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
featureLayer is unused.
Hi here I am, sorry for the noise and thank for your work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice, thanks for this useful addition, @enricofer! I am going to merge now...
My bad, it seems that the automatic checks did not start. By running the tests locally (npm run test
) I found an issue with a test:
1) onMapClick sets correct data if no we have features found
infoclick/InfoClickWin.vue methods
TypeError: Cannot read properties of null (reading 'foo')
at Context.eval (webpack://wegue/./tests/unit/specs/components/infoclick/InfoClickWin.spec.js?:86:31)
Could you please have a look at this @enricofer ? If you need some help with this, please let us know.
I had a quick look, what is wrong with the test: The line https://github.com/wegue-oss/wegue/blob/master/tests/unit/specs/components/infoclick/InfoClickWin.spec.js#L88 and following need to be changed to map.forEachFeatureAtPixel = () => {
vm.features.push([feat, layer]);
}; so the mock still reflects the current code. |
Hi @enricofer , do you see a chance that you push this forward? No fronts, this is not meant to put pressure on you. I know everybody has a lot to do. But this feature is too cool, to hang around here ;-) If you currently do not have time to push this forward, which is no problem, I'd happily take over here and finish this. Thanks for any feedback and your great work so far! |
I apologize at that moment I didn't understand the meaning of the code and I forgot to update the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@enricofer no need to apologize, all good here 👍 Thanks for your ongoing effort.
LGTM now, I am going to merge.
The pull request is about a new infoclick feature allowing to browse between multiple features detected on map visible layers