search

inocan-group / vue3-google-map

A set of composable components for easy use of Google Maps in your Vue 3 projects.
https://vue3-google-map.com
MIT License
264 stars 51 forks source link

Add vModel to infoWindow #151

Closed Tofandel closed 9 months ago

Tofandel commented 11 months ago

This would solve #143

Disclaimer: this has not been tested, so do not merge yet

HusamElbashir commented 11 months ago

If we add this feature I'd like it to use an explicit v-model:open. That way it isn't ambiguous and we could have others added as well in the future such as v-model:content etc.

randomnessdev commented 10 months ago

Will this PR be merged soon ?

Tofandel commented 10 months ago

@randomnessdev I didn't yet have time to test the PR, so it depends on what you mean by soon, within the month maybe, within 2 weeks, unlikely, unless you want to give a hand and test the PR to speed up the process and make sure the v-model is synced in all cases and doesn't do some weird things

@HusamIbrahim Having an unnamed v-model doesn't remove the ability to add other named v-models in the future

The implicit convention is to use v-model for the obvious managed state (which the component itself can obviously modify, for example for inputs it's the value, for modals it's open, for buttons it's active, only very complex components ever require multiple v-models and for them it makes sense to have only named vModel), The info window only has the one obvious state open/closed.

Naming it v-model:open would make it less intuitive to use and require looking up the documentation to know the name, because every developer would just assume straight away it's v-model

So for me, keeping it the default v-model ensures it's an intuitive component and if we ever need other less obvious v-model, we could always still use v-model:position

HusamElbashir commented 10 months ago

@Tofandel Fair enough do let me know when you think this is ready

yankeeinlondon commented 9 months ago

Thanks @Tofandel; going forward can you get in the habit of signing all commits? It's a good practice and dead simple once you invest the initial "and how do I do that" effort :)

Tofandel commented 9 months ago

@yankeeinlondon I was still testing out the PR, there is one type issues that will likely prevent the build

yankeeinlondon commented 9 months ago

Ok @Tofandel ; sorry I jumped the gun there ... multitasking (poorly)

Tofandel commented 9 months ago

Okay, I finished testing, the good news is it was working, so only the missing type was the issue, I also added "prepare" to the scripts so that the package could be installed from git

My bad for not marking as draft

HusamElbashir commented 9 months ago

@Tofandel Sorry for the mishap. Can you open a new PR and link this one to preserve the conversation history?

Tofandel commented 9 months ago

Sure will do, but now that it's rebased on master, there was a recent fastdom addition which is not in the dependencies of the lib

Ah I see it's reverted as well already