use single info point that is extendable

OwnWeb / map_elevation

A widget to display elevation of a track (polyline) like Leaflet.Elevation

MIT License

28 stars 9 forks source link

use single info point that is extendable #1

Closed moovida closed 4 years ago

moovida commented 4 years ago

Hi @tuarrep , thanks for this amazing plugin, I love it :-)

In this pull request I would like to make a proposal to use hoverpoints that:

extend LatLng so that the point list can be used both in the polyline and in the Elevation
change the ElevationHoverNotification to a InfoHoverNotification, and pass that as hover notification instead of only the coordinate. That would allow to extend that InfoPoint and pass through it also other information like progressive distance, speed and timestamp.

I hope you like the idea.

tuarrep commented 4 years ago

Thank you for this PR!

I need to review it deeper later, but here some first impressions.

I'm convinced by the InfoPoint class. It's smarter than mine.

I'm not convinced by the name. "Info" is too vague. What about ElevatedLatLng?

I don't understand the benefits of the changes on the notification. If I've well understood, it's just a name change.

BTW if it's really needed, it should be a second PR to split responsibilities and allow reverting if needed.

Thanks again for your interest.

moovida commented 4 years ago

I'm convinced by the InfoPoint class. It's smarter than mine.

ok, that is great.

I'm not convinced by the name. "Info" is too vague. What about ElevatedLatLng?

I chose info (even if I am also not really happy about it), because it can carry anything. But then you are right. In fact downstream apps will anyways extend the ElevatedLatLng if they add more information. I agree, most probably then your original name would be best to keep?

I don't understand the benefits of the changes on the notification. If I've well understood, it's just a name change.

Yes, I changed the name, and after the discussion above, I think it should stay as it was. The important thing is that the notification carries the complete point and not only the LatLng information.

I could redo the PR reflecting this discussion. What do you think?

moovida commented 4 years ago

Closing this pull request and opening a new one.

tuarrep commented 4 years ago

Just let this one open for the new ElevationPoint, remove the notification renaming

tuarrep commented 4 years ago

To prevent BC we can leave the point named ElevationPoint.

moovida commented 4 years ago

@tuarrep I had made the new pull request in order to not revert changes in this one (I am not sure that it would be so readable). Would you mind give a quick check to the other PR? I think it reflects almost everything you commented.

I would have to add as per your comments:

Add a getter latLng which return the current instance for backward compatibility
Add a doc comment that deprecates it

tuarrep commented 4 years ago

OK.

Closing in favor of #2