symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
857 stars 315 forks source link

[Map] Google map ID config when using a marker #2306

Closed Nayte91 closed 2 weeks ago

Nayte91 commented 3 weeks ago

Hi there,

I'm playing around with UX map with my Google account, and while I'm following a simple example from doc, I get an unexpected behavior;

#[Route('/event/{id}', name: 'app_test')]
public function test(EventETT $event): Response
{
        $point = new Point(48.858370, 2.294481);

        $map = (new Map('default'))
            ->zoom(10)
            ->addMarker(new Marker($point))
            ->center($point)
        ;

        return $this->render('skeletons/event/show.html.twig', [
            'event' => $event,
            'map' => $map
        ]);
}

I think this is a very simple example based on docs, if I didn't made any mistake, I get the following behavior:

image

Google Maps can't load correctly on this page. in the canvas, and The map is being initialized without a valid map ID, which prevents the use of advanced markers. Well, OK, let's do this; I created a map ID on my cloud console, and then... I don't really know how to load it in my app. Here's the official doc about it.

So here I am: did I made a mistake if I get stuck like this? If so, can I improve the doc to help future devs to not get stuck as me? If I didn't made a mistake, is there an elegant, already provided way to add this map ID? I expect a en variable the same way than GOOGLE_MAPS_API_KEY but where to mix them up to create the final url with map ID? I suppose it's related to UX_MAP_DSN but before going deeper and maybe propose an update here, I prefer asking here, because it's a veeery basic step where I'm stuck, so I wonder if there is an already implemented way of dealing with this. And if so, I would add it to the doc!

smnandre commented 3 weeks ago

You can see it in the UX Map Google Maps documentation: https://github.com/symfony/ux-google-map

$googleOptions = (new GoogleOptions())
    ->mapId('YOUR_MAP_ID')
    // ... 
;

// Add the custom options to the map
$map->options($googleOptions);
Kocal commented 3 weeks ago

Maybe we can add a check to detect if mapId is empty when it should have been filled...?

Nayte91 commented 3 weeks ago

Yay thank you, it works! :tada: Some more points below:

smnandre commented 3 weeks ago

: is it a thing to have documention not displayed on the SF website, and to be important in a deported file like this? I mean, I always first (and only 🤦 ) look at main site doc, so I may regularily miss some important info.

It is the same thing for every bridge:

.. but something we plan to change before the end, with dedicated pages on the website

smnandre commented 3 weeks ago

On the implementation side, having the lib that will be able, by default, to read a GOOGLE_MAP_ID env var instead of pasting/creating a whole config to inject it into the requested service could be useful, the exact same way the API KEY is handled. Then maybe allowing the GoogleOptions::mapId field to override the default conf, if needed?

This is a very good idea

smnandre commented 3 weeks ago

Should I add this info in the main ux map page/ in a new SF doc page (dedicated to google map connector for example)?

We can probably add a "tip" admonition here before the table, to tell user to read the dedicated documentation of their renderer they want to use

https://symfony.com/bundles/ux-map/current/index.html#available-renderers

Kocal commented 3 weeks ago

Indeed, things can be improved here and also in the symfony/ux-google-map recipe. I'm gonna work on it. (EDIT: not now)

Thanks for the suggestion :)

But, I'm not 100% fan about using a new dedicated env var, can't we simply add an option mapId to the UX_MAP_DSN env var? Something like google://GOOGLE_MAPS_API_KEY@default?mapId=...?

Nayte91 commented 3 weeks ago

Hello Kocal,

Maybe we can add a check to detect if mapId is empty when it should have been filled...?

What do you think? Currently when you don't have a setted mapId and try to display a Marker, JS raise an error with a pretty understandable message. Do you think about raising a PHP error/warning before rendering?

Indeed, things can be improved here and also in the symfony/ux-google-map recipe. ~I'm gonna work on it.~ (EDIT: not now)

Thanks for the suggestion :)

My pleasure! If you plan to change anything, do you need any help on documentation? There's high chance you will be 20x times better & faster on code side, but I may tweak 1 line or 2 on the doc side as smnandre hinted. Assuming how I experienced my use of ux-map with Google maps, having the possibility to set the mapId on env is a welcomed enhancement, but the rest of my mistakes are documentation related. I'll try something on this department.

But, I'm not 100% fan about using a new dedicated env var, can't we simply add an option mapId to the UX_MAP_DSN env var? Something like google://GOOGLE_MAPS_API_KEY@default?mapId=...?

I don't know what are the best practices on this side, or what already exist anywhere else on SF. I just have the doctrine example where you stack important info on a one liner where it could also be defined one by one:

###> basic doctrine one-liner ###
DATABASE_URL="mysql://db_user:db_password@127.0.0.1:3306/app?serverVersion=8&charset=utf8mb4"
###< basic doctrine one-liner ###

###> same vars but destructured ###
DATABASE_DRIVER=mysql
DATABASE_VERSION=8
DATABASE_CHARSET=utf8mb4
DATABASE_HOST=127.0.0.1
DATABASE_PORT=3306
DATABASE_NAME=app
DATABASE_USER=db_user
DATABASE_PASSWORD=db_password
###< same vars but destructured ###

I personally prefer the later, so please allow me to advocate a bit for it:

So are my args, but at the end of the day you're more experienced and used to good practices, so I have a total confidence about your choice.

Kocal commented 2 weeks ago

Hi @Nayte91, thanks for your words and sorry for the late reply :)

I don't think we will add a new option to the DSN google://GOOGLE_MAPS_API_KEY@defaut?... for the reason that those options are not related to the Map itself but to the provider Google Maps, by that, I mean options defined by @googlemaps/js-api-loader (apiKey, language, region, ...).

I don't think we will add a new env variable like GOOGLE_MAPS_DEFAULT_MAP_ID in the UX Map package, because it would be too magical and I believe it does not makes real sense.

What about adding a new configuration like this?:

# config/packages/ux_map.yaml
ux_map:
    # https://symfony.com/bundles/ux-map/current/index.html#available-renderers
    renderer: '%env(resolve:default::UX_MAP_DSN)%'
    google_maps:
        default_map_id: 'your map id'

If developpers really want to use an env var to configure the default map id, they could do it like that:

# config/packages/ux_map.yaml
ux_map:
    # https://symfony.com/bundles/ux-map/current/index.html#available-renderers
    renderer: '%env(resolve:default::UX_MAP_DSN)%'
    google_maps:
        default_map_id: '%(env:GOOGLE_MAPS_DEFAULT_MAP_ID)%'

and

# .env
GOOGLE_MAPS_DEFAULT_MAP_ID=...

WDYT?

Kocal commented 2 weeks ago

For the ux_map.google_maps.default_map_id, it would be super cool to add a Symfony recipe that automatically add those lines when you install symfony/ux-google-map

Nayte91 commented 2 weeks ago

What about adding a new configuration like this?:

This is the way; it's explicit, centralized with other configs, and easy to deport as an env var. For what it's worth, you have my blessing for this. Same for recipe, but I don't know much on how it works.

I saw that smnandre is working on ux map documentation, I'll check for his final version to add up some paragraphs where I struggled.

smnandre commented 2 weeks ago

Hey @Nayte91! It's been merged so it's all ready for you :)

And .. thanks ;)

Kocal commented 2 weeks ago

Will be available in the next release