ux icon indicating copy to clipboard operation
ux copied to clipboard

[Map] Create Map component

Open Kocal opened this issue 1 year ago • 6 comments

Q A
Bug fix? no
New feature? yes
Issues Fix #38
License MIT

Hi :)

This PR is a proposal for #38 a replacement for #39.

Symfony UX Map is a new Symfony UX component that makes it easy to create, customize and use interactive JavaScript maps.

Currently, only the Google Maps and Leaflet providers are supported.

For each provider, there is:

  • a dedicated configuration
  • a dedicated API with its own specific features
  • a dedicated Stimulus controller
  • a lot of tests

Here an example to render a Google Maps, with a custom MapId, custom center and zoom, some markers and info windows:

    public function index(MapFactoryInterface $mapFactory): Response
    {
        /** @var GoogleMaps\Map $mapGoogle */
        $map = $mapFactory->createMap('google_maps_map_1');
        $map
            // All following methods are optional
            ->setMapId("2b2d73ba4b8c7b41")
            ->setCenter(new LatLng(46.903354, 1.888334))
            ->setZoom(6)
            ->addMarker($paris = new GoogleMaps\Marker(position: new LatLng(48.8566, 2.3522), title: 'Paris'))
            ->addMarker($lyon = new GoogleMaps\Marker(position: new LatLng(45.7640, 4.8357), title: 'Lyon'))
            ->addMarker(new GoogleMaps\Marker(position: new LatLng(43.2965, 5.3698), title: 'Marseille'))
            ->addMarker(new GoogleMaps\Marker(position: new LatLng(43.7102, 7.2620), title: 'Nice'))
            ->addMarker(new GoogleMaps\Marker(position: new LatLng(47.2184, -1.5536), title: 'Nantes'))
            ->addInfoWindow(new GoogleMaps\InfoWindow(
                headerContent: '<b>Paris</b>', 
                content: "Capitale de la France, est une grande ville européenne et un centre mondial de l'art, de la mode, de la gastronomie et de la culture.",
                marker: $paris,
                opened: true,
            ))
            ->addInfoWindow(new GoogleMaps\InfoWindow(
                headerContent: '<b>Lyon</b>', 
                content: 'Ville française de la région historique Rhône-Alpes, se trouve à la jonction du Rhône et de la Saône.', 
                marker: $lyon
            ))
            ->enableStreetViewControl(false)
            ->enableMapTypeControl(false)
            ->setFullscreenControlOptions(new GoogleMaps\FullscreenControlOptions(
                position: GoogleMaps\ControlPosition::BLOCK_START_INLINE_START,
            ))
            ->setZoomControlOptions(new GoogleMaps\ZoomControlOptions(
                position: GoogleMaps\ControlPosition::BLOCK_START_INLINE_END,
            ));
        ;
        
        return $this->render('home/index.html.twig', [
            'map' => $map,
        ]);
    }

The Twig code:

{{ render_map(map_google, { style: 'height: 700px; width: 1024px; margin: 10px' }) }}

It gives you this interactive Google Maps map:

image

~~I'm opening the PR this morning, and I'll try to comment on some of my choices as soon as possible before you start the review :)~~

Kocal avatar Jun 24 '24 07:06 Kocal

Very useful component :)

seb-jean avatar Jun 24 '24 10:06 seb-jean

That's awesome ! Thank you for this PR ! I checked my projects with Google Maps integration, there is some features missing here :

  • allow to set a custom icon on Marker
  • allow to customize map styles (styles parameter as array ?)
  • on some projects I also use @googlemaps/markerclusterer. Not sure if you should include it in ux/maps but I'll do a doc PR with explanations if you want

simondaigre avatar Jun 24 '24 12:06 simondaigre

Hi @simondaigre, and thank you! :)

allow to set a custom icon on Marker

It's already on my list! I will need this feature aswell for my website where I use custom marker icons: image

But I don't wanted to do too much things in a single PR. Implementing PinElement PHP-side is a small challenge which I think can already be done user-side with event listeners.

allow to customize map styles (styles parameter as array ?)

I've started to implement the API/configuration for map styles (with some classes and enums), but I've finally removed it when I knew about Cloud-based maps styling. You can pass the mapId or call ->setMapId('...') to customize your map styles.

on some projects I also use @googlemaps/markerclusterer. Not sure if you should include it in ux/maps but I'll do a doc PR with explanations if you want

I think we would not include it in Symfony UX Map by default, a documentation would be enough IMO :)

Kocal avatar Jun 24 '24 13:06 Kocal

(i will make a big review this week-end ;) )

smnandre avatar Jun 26 '24 15:06 smnandre

A first comment before touching the real work :)

It is not possible to use Google Map without explicit consent of the user in Europe, California i think, Japan maybe, Australia too

You are of course not responsible of this, but i think some use case or implementation consequences should be discussed now

  • can we expose some hook to load scripts & maps on trigger (can be the CMP or GMT for instance) ?
  • can we ease the privacy compliance in any way (because as we discussed a few times with @WebMamba this is probably where some good DX can make a big difference) ?

smnandre avatar Jun 27 '24 04:06 smnandre

... 😮‍💨 , but yeah you're right, thanks for pointing it out.

I think the easiest way to do that is to:

  • never load Google Maps maps implicitly, or at least makes it configurable server-side
    • when calling render_map(...), we won't render data-controller attribute but something else (e.g.: data-controller-wait-for-consent or something more related to Symfony UX Map), so the GoogleMaps Stimulus controller & Google Maps API won't be loaded
  • provide a global function like loadSymfonyUxGoogleMaps():
    • which can be easily created on-the-fly by {{ ux_map_script_tags() }}
    • calling it will rename data-controller-wait-for-consent to data-controller, which will load the Stimulus controller & Google Maps API
  • and let the developper execute loadSymfonyUxGoogleMaps() when needed

For example with the Didomi CMP:

<script>
    window.didomiOnReady = window.didomiOnReady || [];
    window.didomiOnReady.push(() => {
        function loadGoogleMapsIfConsentGiven() {
            const googleMapsPurposeId = '...';
            const googleMapsVendorId = '...';

            const userStatus = Didomi.getUserStatus();
            const enabledPurposeConsent = userStatus.purposes.consent.enabled;
            const enabledVendorConsent = userStatus.vendors.consent.enabled;

            if (enabledPurposeConsent.includes(googleMapsPurposeId) && enabledVendorConsent.includes(googleMapsVendorId)) {
                window.loadSymfonyUxGoogleMaps();
            }
        }

        if (Didomi.shouldConsentBeCollected()) {
            window.didomiEventListeners = window.didomiEventListeners || [];
            window.didomiEventListeners.push({
                event: 'consent.changed',
                listener: function (event) {
                    loadGoogleMapsIfConsentGiven();
                }
            });
        } else {
            loadGoogleMapsIfConsentGiven();
        }
    });
</script>

WDYT?

Kocal avatar Jun 27 '24 06:06 Kocal

Hey @smnandre and @kbond, here is my 2nd iteration to simplify the package a lot, thanks to your reviews :)

You can find an example in the PR description, but to resume:

  • One default provider is configured through ux_map.provider, it can either be Leaflet or Google Maps
  • Multiple providers or maps can not be configured anymore in the yaml file
  • You can easily create your own ProviderInterface in your application, and pass its instance to $mapFactory->create()
  • A tons of tests have been added (especially the ones for AssetMapper and ImportMap support)
  • The documentation has been updated

Kocal avatar Jul 06 '24 23:07 Kocal

Thanks for the review, I've applied your minor suggestions, I will look into the other ones later :)

Kocal avatar Jul 07 '24 19:07 Kocal

❤️

smnandre avatar Jul 10 '24 23:07 smnandre

Hey everyone! Here is the 3rd iteration of Symfony UX Map :)

To resume:

  • Bridges have been introduced, they will be available under symfony/ux-map-google and symfony/ux-map-leaflet. They ship their own renderer, map options, and Stimulus controller to create the map, markers, and infoWindows
  • Yarn workspaces have been re-configured, some Yarn commands have been adapted, and the CI has been adapted
  • For the Leaflet bridge, there is no more AssetMapper/ImportMap hackings 🎉 , now we simply configure on-the-fly the Marker icon by using an inlined SVG
  • The documentations have been rewritten
  • The MapFactory have been deleted (use new Map() instead), and the rendering system have been completely rewritten, the Map does not contain renderer reference (previously "provider") nor HTML attributes

The PR description have been updated aswell.

Happy reviews :eyes:

Kocal avatar Jul 18 '24 23:07 Kocal

@Kocal excellent work! I hope this is merged very soon.

Not sure if this was discussed before, but about the proposed LatLng class name, I searched a bit about this and:

Google Maps is arguably the most popular, so lat and lng are very common for developers and the proposed LatLng is fine.

But, what would you think of using LatLon instead? I find it easier to understand, what about you?

javiereguiluz avatar Jul 30 '24 10:07 javiereguiluz

Thanks @javiereguiluz :)

The LatLng class name was not discussed before, but during my first implementation I've thought about Position instead. Something usable like Position::latLng(5, 40) or new Position(latitude: 5, longitude: 40). But it's too abstract IMO, to me a position can be defined by an address, a city, a country etc...

I think aswell LatLng is the most common/popular naming, thanks to Google Maps. lon feels weird for me, and I really don't like the possible LatitudeLongitude class name.

I really like LatLng, but if this really needs to be changed, I can suggest Point.

WDYT?

Kocal avatar Jul 30 '24 18:07 Kocal

(Waiting for #2015 to be merged before rebasing this one)

Kocal avatar Jul 30 '24 18:07 Kocal

but during my first implementation I've thought about Position instead.

What about Point?

to me a position can be defined by an address, a city, a country etc...

Is this type of position out of scope of this package/bridges?

kbond avatar Jul 30 '24 19:07 kbond

but during my first implementation I've thought about Position instead.

What about Point?

If we really need to change LatLng, we can go for Point yes.

to me a position can be defined by an address, a city, a country etc...

Is this type of position out of scope of this package/bridges?

Yes totally, Google Maps and Leaflet (and maybe a tons of other providers) use lat/lng combo

Kocal avatar Jul 30 '24 20:07 Kocal

What about Coordinates ? Would be a bit longer, but probably more precise ?

(Point is a good choice too for me)

smnandre avatar Jul 30 '24 20:07 smnandre

Location can be a good choice too, but feel a bit "wider"

smnandre avatar Jul 30 '24 20:07 smnandre

Coordinates is nice too

PS: Biome found really interesting issues 😍 image

Kocal avatar Jul 30 '24 20:07 Kocal

Yes totally, Google Maps and Leaflet (and maybe a tons of other providers) use lat/lng combo

Sorry, I meant, can the google/leaflet api use an address for a marker instead of lat/lang?

kbond avatar Jul 30 '24 20:07 kbond

Yes totally, Google Maps and Leaflet (and maybe a tons of other providers) use lat/lng combo

Sorry, I meant, can the google/leaflet api use an address for a marker instead of lat/lang?

I wasn't clear enough but don't worry I understood :)

So, Google Maps/Leaflet only use lat/lng combo for positioning a marker. If you want to use an address for a marker, first you need to geocode this address to get lat/lng!

Kocal avatar Jul 30 '24 20:07 Kocal

If you want to use an address for a marker, first you need to geocode this address to get lat/lng!

Got it!

Point is a term used by most databases for this so feels like the right choice. Coordinates is a bit more descriptive but longer. I'd prefer Point but I don't have a strong opinion.

kbond avatar Jul 30 '24 20:07 kbond

We can go for Point if many people here wants to :)

Kocal avatar Jul 30 '24 20:07 Kocal

I've just changed LatLng for Point :)

Kocal avatar Jul 31 '24 14:07 Kocal

Do we have a list of things left to do here before making this mergeable?

Or is this fully finished and just needs a final review?

Thanks!

javiereguiluz avatar Aug 06 '24 13:08 javiereguiluz

Hi, I don't have more things to do, I've already processed the ~250+ comments :D

Final reviews are welcome of course, but I think we planned to merge this PR soonly so we can iterate on it (especially the documentation), cc @kbond @smnandre

Kocal avatar Aug 06 '24 14:08 Kocal

It's merged now! 🎉🎉🎉

Hugo, infinite thanks for contributing this amazing new component 🙇🙇🙇 and thank you all for the nice discussion and review that you did here.

Now, let's test it in real apps, let's iterate on it and let's add good docs for the community. Thanks!

javiereguiluz avatar Aug 07 '24 12:08 javiereguiluz

Thanks @javiereguiluz :)

But we still need to set-up git repositories for UX Map bridges (Google and Leaflet), AFAIK only Fabien can do that..? Do you think we can get in touch with him? 🙏

Thanks!

Kocal avatar Aug 07 '24 13:08 Kocal

@Kocal Can you list what needs to be done? Based on that, I will do the magic ;)

fabpot avatar Aug 07 '24 13:08 fabpot

So quick! :D

We will need dedicated repositories for:

  • https://github.com/symfony/ux/tree/2.x/src/Map
  • https://github.com/symfony/ux/tree/2.x/src/Map/src/Bridge/Google
  • https://github.com/symfony/ux/tree/2.x/src/Map/src/Bridge/Leaflet

So packages symfony/ux-map, symfony/ux-map-google and symfony/ux-map-leaflet can be published on Packagist, and so downloadable by people.

And like other Symfony components, bridges source code should not be present in symfony/ux-map repo/package.

Thanks :)

Kocal avatar Aug 07 '24 13:08 Kocal

@Kocal All done. Can you double-check that everything has been configured properly?

fabpot avatar Aug 07 '24 14:08 fabpot