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

Not accurate prop type for Marker component #383

Open
karol-bujacek opened this issue May 24, 2021 · 4 comments
Open

Not accurate prop type for Marker component #383

karol-bujacek opened this issue May 24, 2021 · 4 comments

Comments

@karol-bujacek
Copy link

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
Copy link

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
Copy link
Author

karol-bujacek commented May 25, 2021

@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
Copy link

@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
Copy link
Contributor

iBobik commented Jun 10, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants