search

vinayakkulkarni / v-mapbox

Vue-ish way for interacting with map(box|libre)-gl-js 🧭
https://v-mapbox.netlify.app/
MIT License
187 stars 45 forks source link

Not accurate prop type for Marker component #383

Open karol-bujacek opened 3 years ago

karol-bujacek commented 3 years ago

In the current code the type of coordinates prop is set to Array and this is not correct. Why?

Mapbox-gl is accepting for marker coordinates a LngLatLike object, which is described as ‘an array of two numbers representing longitude and latitude, or an object with lng and lat or lon and lat properties.’ (cited from https://docs.mapbox.com/mapbox-gl-js/api/geography/#lnglatlike)

That mean that v-mapbox code is a far more restricting than the library it is using. This forces us, developers, to convert an object to an array, i.e. to use more primitive attributes than can be used. Especially in the case of longitude and latitude it is error prone – you can easily misorder the coordinates and use [latitude, longitude] instead of [longitude, latitude] (I’ve almost trained myself to check that other location immediately when my markers are not present – then started to use objects instead of arrays every time it is possible :-) ).

Maker-Mark commented 3 years ago

I'm not sure about this. We should support both if we can. When it comes to working with LngLat, it's easy to mix it up. Especially if you wrote application logic that uses the other order arround.

At the same time, I dont think it makes sense to send in an object as a prop. Instead, it would make more sense to send two props, lat and long, and have prop validation that ensures both are provided.

karol-bujacek commented 3 years ago

@Maker-Mark , the point is that mapgox-gl is already accepting LngLatLike object and also v-mapbox is working correctly passing an LngLatLike object. The only issue is that an error message in shown in the developer console – ‘you are passing an object, but an array is expected’, etc.

An array with two numbers, longitude and latitude, is also a valid value for LngLatLike object. See the source code

export type LngLatLike = LngLat | {lng: number, lat: number} | {lon: number, lat: number} | [number, number];

(copied from here: https://github.com/mapbox/mapbox-gl-js/blob/50adf1cc14e5aef09099f15c5cb803eaa5f72a48/src/geo/lng_lat.js – repository at version 1.13.1)

Is this in harmony with your words ‘we should support both’?

Maker-Mark commented 3 years ago

@karol-bujacek Ahh you're right. We just need to expect a lat lon like type. Not sure where that would go in this repo.

iBobik commented 2 years ago

Current version of VMarker is written corectly: https://github.com/geospoc/v-mapbox/blob/main/types/markers/VMarker.vue.d.ts#L18