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

Add "action type" select to select-features example #2276

Merged
merged 4 commits into from Jul 3, 2014
Merged

Add "action type" select to select-features example #2276

merged 4 commits into from Jul 3, 2014

Conversation

elemoine
Copy link
Member

Making it possible to choose between two select interactions, one that works on singleclick ("click") and another one that works on mousemove ("hover").

See https://groups.google.com/d/msg/ol3-dev/JWpcS_31zeg/UTfypkXqBl0J.

@probins
Copy link
Contributor

probins commented Jun 29, 2014

worth adding an ol.events.condition.mouseMove? That would make it a bit easier

@elemoine
Copy link
Member Author

Yes, possibly.

Also, by default, the select interaction uses a singleClick condition. This means that there's some delay between the click and the actual feature selection.

We could also add ol.events.condition.click for people who want the feature selection to be more responsive, and don't care if feature selection triggers on double-clicks.

We could even make the interaction use ol.events.condition.click by default. I think I tried that in the past but ran into problems. I can't remember what these problems were. I'll maybe give it another try.

@ahocevar
Copy link
Member

See #1946 for an old pull request of mine that adds a click condition. The reason why singleclick is used in the examples is to avoid selection on doubleclick-zoom. Having said that, I never was a fan of this, and this is why I created the above pull request to use click in my own applications, where doubleclick zoom is disabled.

@elemoine
Copy link
Member Author

Thanks @ahocevar. See my new commits.

@probins
Copy link
Contributor

probins commented Jun 30, 2014

I would add a note to select-features.html explaining the difference between click and single-click. In this example, large parts of the viewport are covered by features; if you just have a few points, the click/dbl-click issue doesn't really arise, as people are unlikely to dbl-click on a point anyway.

@marcjansen
Copy link
Member

I think the new conditions miss the @todo api annotation, @elemoine.

I agree with @probins that the types in the examples should have a description.

Btw. when assigning the onchangehandler, do we need the wrapping function? Does reading out the value attribute of the select-element work everywhere, i.e. in every supported browser?

Otherwise this looks great to me.

@probins
Copy link
Contributor

probins commented Jul 2, 2014

I think the new conditions miss the @todo api annotation

well spotted! how come Travis doesn't consider this an error, if they're not exported?

@marcjansen
Copy link
Member

AFAICT, the @todo annotation are relevant only to jsdoc.

@elemoine
Copy link
Member Author

elemoine commented Jul 3, 2014

AFAICT, the @todo annotation are relevant only to jsdoc.

No, it also makes the symbol exportable.

@elemoine
Copy link
Member Author

elemoine commented Jul 3, 2014

well spotted! how come Travis doesn't consider this an error, if they're not exported?

Because we don't de-reference ol.events.condition.click. I was just passing undefined values to the select interactions.

@elemoine
Copy link
Member Author

elemoine commented Jul 3, 2014

Review comments taken into account. Please tell me if my changes do address all of your comments. Thanks.

@elemoine elemoine added this to the v3.0.0-gamma.2 milestone Jul 3, 2014
Making it possible to choose between three select interactions, one that works on "singleclick", one that works on "click", and one that works on "mousemove".
@probins
Copy link
Contributor

probins commented Jul 3, 2014

lgtm

elemoine pushed a commit that referenced this pull request Jul 3, 2014
Add "action type" select to select-features example
@elemoine elemoine merged commit 450d42a into openlayers:master Jul 3, 2014
@elemoine elemoine deleted the select branch July 3, 2014 10:36
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.

None yet

4 participants