Add point selection type to selection package

[HenFo]

I extended the selection package to allow the user to select features using a single click on the map.
It is fully compatible with the extent selection and all sources that already support it.

[changeset-bot]

🦋 Changeset detected

Latest commit: a8e63fc

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[arnevogt]

@HenFo
Thanks for your PR, this is a welcome addition.

Overall, this already looks quite good.
My only concern are the two ToolButtons for switching the selection mode. Because previously we had no button at all this would break the layout of most existing apps.
In my opinion the best solution would be:

• introduce a property for the Selection component to make the available selection modes configurable
  • allow a list of selection modes
    • if both selection modes are configured (default) show both tool buttons
    • if only one selection mode is configured show no button (this matches the current behavior)

a few minor TODOs:

• add unit tests for the new functionality
• add a changeset which describe the changes
• adjust the documentation

If you have any questions, please feel free to ask, I will be happy to assist.

[HenFo]

I made the requested changes. However, I am not sure about further tests as I just started working with react and trails, so I am not that experienced with UI tests.
With changeset you mean an entry in the Changelog?

I extracted the common functions into a helper file, since inheritance would only add complexity for functions that should be some kind of mixin rather than class bound.

[arnevogt]

@HenFo
Thanks for the updates.

Basically, the functionality is fine. Though, I have one concern.
The selection component does not react to changes of the selectionMethods property.
For example, clickling the "switch" Button has no effect.

const [selKind, setSelKind] = useState<SelectionKind>("extent");
<Selection
    sources={sources}
    selectionMethods={selKind}
    onSelectionComplete={onSelectionComplete}
    onSelectionSourceChanged={onSelectionSourceChanged}
/>
</TitledSection>
<Button onClick={() => setSelKind("point")}>switch</Button>

In React these "none-reactive" components are sometimes considered to be a anti-pattern.
I think this could be improved by separating available selection methods (property) and active selection method (internal state).

[HenFo]

@arnevogt didn't see that! Hope this will come with some more experience ;)

Basically, the functionality is fine. Though, I have one concern. The selection component does not react to changes of the selectionMethods property. For example, clickling the "switch" Button has no effect.

I also noticed that I created ambiguous typing with the name SelectionKind, as it already exists.
I don't want to change the existing one, since the point selection is implemented as an extent selection and the existing one is used for the SelectionSource interface, which in fact still receives an extent.
So I am not sure how to name things, I renamed my SelectionKind to SelectionMethod. I.e. SelectionKind is "external" while SelectionMethod is used internally and for the Selection-Component.
Any thoughts on that?

[arnevogt]

@HenFo
regarding the naming:
I prefer selectionMethod (instead of selectionKind) in general.
I think availableSelectionMethods for the component property and activeSelectionMethod for the internal state are descriptive names.

[github-actions]

PR Preview Action v1.4.7
🚀 Deployed preview to https://open-pioneer.github.io/trails-pr-previews/openlayers-base-packages/pr-previews/pr-372/
on branch gh-pages at 2024-12-02 20:27 UTC