feat: add hit detection to `Polyline`s #1728

ignatz · 2023-11-10T21:02:02Z

No description provided.

JaffaKetchup · 2023-11-24T15:12:35Z

@ignatz With all of that decided, it'd be great if you could fix the conflicts here, and just add Hugo's name to the top of the new code (in a comment or something) :)

JaffaKetchup · 2023-11-24T15:15:02Z

Also just need to confirm how this PR will interact with #1704, and which should be merged first.

ignatz · 2023-11-24T16:21:02Z

and just add Hugo's name to the top of the new code (in a comment or something) :)

Help me out. What would you like me to write/credit? I'll put there whatever you'd like me to put.

It's not like FM generally credits authors (which I like) nor is this code based on Hugo's code. I feel a bit silly to mention that there used to be an unrelated plugin that did something similar.

JaffaKetchup · 2023-11-24T17:42:37Z

Yeah, we don't normally credit code unless we're absorbing it. In this case, I feel like we should thank Hugo, just something like "Thanks to Hugo H for maintaining tappability until v7, now maintained here for flexibility".

ignatz · 2023-11-24T18:33:30Z

Sounds reaonsable if there was some sort oft absorbing happening. "Thanks to Hugo for maintaining tappability until v7, now maintained here for flexibility" feels misleading. It seems to imply a lineage where there's none. I also don't quite understand the flexibility argument. If at all, it might be worth pointing out to users that these are entirely separate implementations that behave semantically differently.

josxha · 2023-11-24T19:34:22Z

I would agree that adding it on top of the implementation could be misleading. It makes sense to me if we want to hint that code got integrated to show the copyright but in this case it's different. 🤔
Maybe the migration docs, release notes and/or changelog fits that message better?

JaffaKetchup · 2023-11-24T21:36:46Z

Ok, sure, we'll leave it out of here then, and add it to the docs site.

josxha

Haven't experienced any bugs while testing. Thanks @ignatz for adding a nice showcase to the example app too.
Could you check my comments?

lib/src/layer/polyline_layer.dart

JaffaKetchup · 2023-11-30T20:22:32Z

Although this is technically non-breaking, it will not be included in v6.1.0, because of its impact on the plugin.

JaffaKetchup · 2023-12-12T18:19:05Z

Hey @ignatz, would appreciate your thoughts if you have any!

ignatz · 2023-12-12T18:56:14Z

Sorry, on what specifically? I did express thoughts before. Would you like me to review the code?

lib/src/layer/polyline_layer.dart

JaffaKetchup · 2023-12-12T19:03:00Z

You're right, I just hoped that I've found a solution that isn't too messy. Just wanted to check with you that you couldn't see any glaring issues with this approach that I've missed.

lib/src/layer/polyline_layer.dart

example/lib/pages/polyline.dart

lib/src/layer/polyline_layer.dart

…f just `tapKey` Cleaned up Fixed bugs surrounding `onTap` variants Fixed bugs and removed unnecessary code surrounding the detection algorithm's duplication of results

Added equality methods to `Polyline`

ignatz

I'm glad we argued. I think I like this better than my original proposal. Gives you all the flexibility w/o much complexity. Definitely let's me minimize my fork even more. Thanks

lib/src/layer/polyline_layer.dart

JaffaKetchup · 2023-12-13T22:24:04Z

Ok, everything is ready for final review now :D

I've also written the website documentation for interactivity: https://docs.fleaflet.dev/v/v7-beta/layers/polyline-layer#interactivity. Let me know if that seems suitable and comprehensive enough.
(When tappable Polygons arrive, we can move this documentation into its own page, and make it less specific to Polylines, assuming we follow the same API model.)

Minor documentation improvements

ignatz

LGTM

lib/src/layer/polyline_layer.dart

example/lib/pages/polyline.dart

josxha

I only found some small suggestions. Up to you how and if you solve them.
Great work @JaffaKetchup and @ignatz!
Tested on windows and web, works like a charm.

lib/src/layer/polyline_layer.dart

JaffaKetchup · 2023-12-15T22:42:46Z

Huge thanks @ignatz! Now on to polygons when you're ready and have had a break from me :D

ignatz force-pushed the polyline_tap branch from 571f008 to ae95b27 Compare November 10, 2023 21:15

JaffaKetchup marked this pull request as draft November 10, 2023 22:26

JaffaKetchup changed the title ~~Add an onTap callback to polylines.~~ [POLYLINE] feat: make Polylines tappable Nov 10, 2023

JaffaKetchup changed the title ~~[POLYLINE] feat: make Polylines tappable~~ [PLINE-DEP] feat: make Polylines tappable Nov 10, 2023

JaffaKetchup modified the milestone: Polyline (Non-Perf) Improvements Nov 10, 2023

ignatz force-pushed the polyline_tap branch from ae95b27 to d4ec207 Compare November 12, 2023 21:34

JaffaKetchup changed the title ~~[PLINE-DEP] feat: make Polylines tappable~~ feat: make Polylines tappable Nov 24, 2023

JaffaKetchup marked this pull request as ready for review November 24, 2023 15:11

JaffaKetchup requested review from ibrierley, mootw, TesteurManiak, josxha and JaffaKetchup November 24, 2023 15:12

JaffaKetchup mentioned this pull request Nov 24, 2023

refactor: use separate painters for each Polyline #1697

Closed

ignatz force-pushed the polyline_tap branch 2 times, most recently from c0b156d to 1433b80 Compare November 24, 2023 22:38

Add an onTap callback to polylines.

8b2b763

ignatz force-pushed the polyline_tap branch from 1433b80 to 8b2b763 Compare November 24, 2023 23:17

josxha linked an issue Nov 26, 2023 that may be closed by this pull request

[FEATURE] Interactive Polygons #385

Closed

josxha requested changes Nov 28, 2023

View reviewed changes

josxha added this to the v7.0 milestone Dec 2, 2023

JaffaKetchup reviewed Dec 12, 2023

View reviewed changes

lib/src/layer/polyline_layer.dart Outdated Show resolved Hide resolved

Fix minor documentation mistake

de17f41

Simplified example changes

88830ff

ignatz commented Dec 12, 2023

View reviewed changes

lib/src/layer/polyline_layer.dart Outdated Show resolved Hide resolved

lib/src/layer/polyline_layer.dart Outdated Show resolved Hide resolved

lib/src/layer/polyline_layer.dart Outdated Show resolved Hide resolved

lib/src/layer/polyline_layer.dart Outdated Show resolved Hide resolved

Use "occlude" instead of "obscure"

f820807

josxha requested changes Dec 12, 2023

View reviewed changes

JaffaKetchup added 3 commits December 12, 2023 21:52

Pass entire Polyline object through in handler callbacks, instead o…

6a80c32

…f just `tapKey` Cleaned up Fixed bugs surrounding `onTap` variants Fixed bugs and removed unnecessary code surrounding the detection algorithm's duplication of results

Merge branch 'master' into polyline_tap

d1f525f

Implement new base-only hit detection API

1fd6e6d

JaffaKetchup requested review from josxha and JaffaKetchup December 13, 2023 13:39

Improved example application

7f586c2

Added equality methods to `Polyline`

ignatz commented Dec 13, 2023

View reviewed changes

Minor improvements from review

908f0dd

JaffaKetchup added 3 commits December 14, 2023 12:50

Remove hitKey and generic typing

7954ba2

Replace hitTolerance with minimumHitbox

403d837

Remove remaining reference to Polyline.hitKey

5eec211

Minor documentation improvements

ignatz commented Dec 15, 2023

View reviewed changes

lib/src/layer/polyline_layer.dart Outdated Show resolved Hide resolved

example/lib/pages/polyline.dart Outdated Show resolved Hide resolved

josxha approved these changes Dec 15, 2023

View reviewed changes

lib/src/layer/polyline_layer.dart Show resolved Hide resolved

lib/src/layer/polyline_layer.dart Show resolved Hide resolved

lib/src/layer/polyline_layer.dart Show resolved Hide resolved

JaffaKetchup added 2 commits December 15, 2023 22:32

Revert polylines type to List from Iterable

3ef7ee3

Paint hover lines on top in polyline example

fb9751d

JaffaKetchup changed the title ~~feat: make Polylines tappable~~ feat: add hit detection to Polylines Dec 15, 2023

JaffaKetchup merged commit b94de88 into fleaflet:master Dec 15, 2023
6 checks passed

JaffaKetchup removed a link to an issue Dec 15, 2023

[FEATURE] Interactive Polygons #385

Closed

JaffaKetchup mentioned this pull request Dec 15, 2023

[FEATURE] Interactive Polygons #385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add hit detection to `Polyline`s #1728

feat: add hit detection to `Polyline`s #1728

ignatz commented Nov 10, 2023

JaffaKetchup commented Nov 24, 2023

JaffaKetchup commented Nov 24, 2023

ignatz commented Nov 24, 2023

JaffaKetchup commented Nov 24, 2023

ignatz commented Nov 24, 2023

josxha commented Nov 24, 2023

JaffaKetchup commented Nov 24, 2023

josxha left a comment

JaffaKetchup commented Nov 30, 2023

JaffaKetchup commented Dec 12, 2023

ignatz commented Dec 12, 2023

JaffaKetchup commented Dec 12, 2023

ignatz left a comment

JaffaKetchup commented Dec 13, 2023 •

edited

Loading

ignatz left a comment

josxha left a comment

JaffaKetchup commented Dec 15, 2023

feat: add hit detection to Polylines #1728

feat: add hit detection to Polylines #1728

Conversation

ignatz commented Nov 10, 2023

JaffaKetchup commented Nov 24, 2023

JaffaKetchup commented Nov 24, 2023

ignatz commented Nov 24, 2023

JaffaKetchup commented Nov 24, 2023

ignatz commented Nov 24, 2023

josxha commented Nov 24, 2023

JaffaKetchup commented Nov 24, 2023

josxha left a comment

Choose a reason for hiding this comment

JaffaKetchup commented Nov 30, 2023

JaffaKetchup commented Dec 12, 2023

ignatz commented Dec 12, 2023

JaffaKetchup commented Dec 12, 2023

ignatz left a comment

Choose a reason for hiding this comment

JaffaKetchup commented Dec 13, 2023 • edited Loading

ignatz left a comment

Choose a reason for hiding this comment

josxha left a comment

Choose a reason for hiding this comment

JaffaKetchup commented Dec 15, 2023

feat: add hit detection to `Polyline`s #1728

feat: add hit detection to `Polyline`s #1728

JaffaKetchup commented Dec 13, 2023 •

edited

Loading