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

implement unselectable features #197

Merged
merged 9 commits into from
Nov 5, 2024
Merged

Conversation

warm-coolguy
Copy link
Member

Summary

  • For the GFI, a method has been added to mark features as unselectable. They are not styled differently, but just ignored. If visible/displayed in another way, styling has to remove them there, too.
  • For the extendedMasterportalapiMarkers in the core, the feature has been doubled; since it is orthogonal to GFI, two configurations are required to get the feature running in this mode, one on the markers, one in the GFI. However, styling has been resolved here, as it is internal to this feature.

Instructions for local reproduction and review

Use the snowbox to check the behaviour. Use cases have been added.

  • GFI (pure): Switch the "Compensation area" layer visible. The feature left of the Flugzeugwerft (left of Finkenwerder) should not be clickable, but the one to its right should be. example1
  • GFI + extendedMasterportalapiMarkers: Switch the "Reports (MML)" layer on. Arbitrarily, features with uneven ID have been deemed unselectable. Check whether the behaviour seems plausible/correct to you.

You may also test GFI on the GeoJSON layer by removing the extendedMasterportalapiMarkers and enableClustering related code. In that case, all pins are visible, but only pins with even number display a GFI window. As in the first case, styling will have to be added later on the layer itself.

Also, please confirm that nothing in the Meldemichel broke.

@warm-coolguy warm-coolguy added the enhancement New feature or request label Oct 8, 2024
@warm-coolguy warm-coolguy self-assigned this Oct 8, 2024
Copy link
Member

@dopenguin dopenguin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I configure featureList, non-selectable features are still selectable. Thus, they also change their styling if bindWithCoreHoverSelect is used.
I would have expected that features that not selectable in the map are not being shown in the featureList. Would you agree?

Edit: Meldemichel seems to be working fine as far as I can tell.

🏓 @warm-coolguy

packages/plugins/Gfi/CHANGELOG.md Outdated Show resolved Hide resolved
packages/core/src/vuePlugins/vuex.ts Show resolved Hide resolved
packages/core/README.md Show resolved Hide resolved
packages/clients/snowbox/src/polar-client.ts Show resolved Hide resolved
warm-coolguy and others added 2 commits October 11, 2024 06:35
apply suggestion "remove misplaced word"

Co-authored-by: Pascal Röhling <[email protected]>
@warm-coolguy
Copy link
Member Author

warm-coolguy commented Oct 14, 2024

If I configure featureList, non-selectable features are still selectable. Thus, they also change their styling if bindWithCoreHoverSelect is used. I would have expected that features that not selectable in the map are not being shown in the featureList. Would you agree?

🏓 @warm-coolguy

Yes, I agree. As discussed before, I'm shoveling the PR over to you and added you as assignee. Should any question about my implementation so far arise, call me anytime.

🏓 @dopenguin

@dopenguin
Copy link
Member

If I configure featureList, non-selectable features are still selectable. Thus, they also change their styling if bindWithCoreHoverSelect is used. I would have expected that features that not selectable in the map are not being shown in the featureList. Would you agree?
🏓 @warm-coolguy

Yes, I agree. As discussed before, I'm shoveling the PR over to you and added you as assignee. Should any question about my implementation so far arise, call me anytime.

🏓 @dopenguin

I've fixed that behaviour in c38065c.
If you want to reproduce it, I've attached a patch with the relevant changes to @polar/client-snowbox.

🏓 @warm-coolguy

@warm-coolguy warm-coolguy self-assigned this Nov 4, 2024
@warm-coolguy
Copy link
Member Author

If I configure featureList, non-selectable features are still selectable. Thus, they also change their styling if bindWithCoreHoverSelect is used. I would have expected that features that not selectable in the map are not being shown in the featureList. Would you agree?
🏓 @warm-coolguy

Yes, I agree. As discussed before, I'm shoveling the PR over to you and added you as assignee. Should any question about my implementation so far arise, call me anytime.
🏓 @dopenguin

I've fixed that behaviour in c38065c. If you want to reproduce it, I've attached a patch with the relevant changes to @polar/client-snowbox.

🏓 @warm-coolguy

🚀 @dopenguin Checked and just works. Nice.

Copy link
Member

@dopenguin dopenguin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I configure featureList, non-selectable features are still selectable. Thus, they also change their styling if bindWithCoreHoverSelect is used. I would have expected that features that not selectable in the map are not being shown in the featureList. Would you agree?
🏓 @warm-coolguy

Yes, I agree. As discussed before, I'm shoveling the PR over to you and added you as assignee. Should any question about my implementation so far arise, call me anytime.
🏓 @dopenguin

I've fixed that behaviour in c38065c. If you want to reproduce it, I've attached a patch with the relevant changes to @polar/client-snowbox.
🏓 @warm-coolguy

🚀 @dopenguin Checked and just works. Nice.

🚀 @warm-coolguy weeeee

@dopenguin dopenguin merged commit 8b112a6 into main Nov 5, 2024
4 checks passed
@dopenguin dopenguin deleted the feature/unselectable-features branch November 5, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants