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 select interaction #277

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

Conversation

ludvigeriksson
Copy link

This pull request adds the select interaction to rlayers.

Following the React spirit described in the readme, I opted to automatically add a layer filter if the interaction is placed inside a vector layer. See examples below. Do you agree with this decision?

In this example only features from the vector layer of which the interaction is placed inside will be selectable.

<RMap>
    <RLayerVector>
        <RSelect onSelect={(event) => console.log(event)} />
    </RLayerVector>
    <RLayerVector />
</RMap>

In this example features from every vector layer will be selectable.

<RMap>
    <RSelect onSelect={(event) => console.log(event)} />
    <RLayerVector />
    <RLayerVector />
</RMap>

I did not find any documentation for contributing, so I'm not sure how examples and tests are set up. If I can get some pointers for this I can try to add these as well.

@mmomtchev
Copy link
Owner

As you may have seen from my Github profile, I am currently unemployed and living on social welfare because of a huge extortion involving the French judiciary and the French police. The object of this extortion is that I accept a job in a company where I will be getting various viagra calls and dicks messages simultaneous with orders from my supervisor - in order to ease the pain of a guy with a serious issue. In order to intimidate me, I am getting simultaneous posts on Github - this PR is simultaneous with a comment on a discussion in the Vercel repository where I frequently get such comments.

Thus said, if you add a proper unit test and fix the failing ones, this will get merged, as making good software is the main reason I am here on Github.

However I insist to also be able to note those occurrences.

@ludvigeriksson
Copy link
Author

Hello Momtchil, and thank you for a quick answer.

I'm sorry to hear about your situation. I must say I am a bit discouraged to contribute to this repository when being met with this tone, as well as the response to the bug report issue I created earlier yesterday. However, if this happened to coincide with other comments you received, I'm sorry about that coincidence, and I hope we can keep it professional going forwards.

I will look at the existing unit tests and match them as well as I can for the RSelect interaction. However, the currently failing unit test seems to be for the RLayerStadia component, which fails on the main branch as well.

I would also like to take this opportunity to say kudos for a very nice implementation of OpenLayers for React. I'm currently working at Vidheim (https://www.vidheim.com, in Swedish only, sorry), which is a newly established company that sells maps and services mostly within the forestry industry, but I've worked in the forestry/GIS industry for over 6 years. During this time I have (actually twice) implemented a very similar (closed source) library, but it never reached quite the coverage that this library has. I especially like how smooth the inheritance worked when implementing a new component, for example the automatic event handlers.

@ludvigeriksson
Copy link
Author

Do you need me to add anything more or are my tests sufficient for getting this merged?

@mmomtchev
Copy link
Owner

@ludvigeriksson can you rebase against the latest main?

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.

2 participants