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

[Map] Create Map component #1937

Merged
merged 1 commit into from
Aug 7, 2024
Merged

Conversation

Kocal
Copy link
Collaborator

@Kocal Kocal commented Jun 24, 2024

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.

The package ships with:

Example

Bridge configuration

# .env
UX_MAP_DSN=leaflet://default
# config/packages/ux_map.yaml
ux_map:
    renderer: '%env(UX_MAP_DSN)%'

Map creation

An example to render a map, custom center and zoom, some markers and info windows:

namespace App\Controller;

use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Attribute\Route;
use Symfony\UX\Map\InfoWindow;
use Symfony\UX\Map\MapFactoryInterface;
use Symfony\UX\Map\Marker;
use Symfony\UX\Map\Point;

final class ContactController extends AbstractController
{
    #[Route('/contact')]
    public function __invoke(): Response
    {
            // 1. Create a new map instance
            $myMap = (new Map());
                ->center(new Point(46.903354, 1.888334))
                ->zoom(6)
            ;
    
            // 2. You can add markers, with an optional info window
            $myMap
                ->addMarker(new Marker(
                    position: new Point(48.8566, 2.3522), 
                    title: 'Paris'
                ))
                ->addMarker(new Marker(
                    position: new Point(45.7640, 4.8357), 
                    title: 'Lyon',
                    // With an info window
                    infoWindow: new InfoWindow(
                        headerContent: '<b>Lyon</b>',
                        content: 'The French town in the historic Rhône-Alpes region, located at the junction of the Rhône and Saône rivers.'
                    )
                ));
    
            // 3. And inject the map in your template to render it
            return $this->render('contact/index.html.twig', [
                'my_map' => $myMap,
            ]);
    }
}

Map rendering

You must call render_map(map) to render the map:

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

It gives you this interactive Leaflet map:

image

@carsonbot carsonbot added Feature New Feature Status: Needs Review Needs to be reviewed labels Jun 24, 2024
@Kocal Kocal force-pushed the feat/38-map-component branch 4 times, most recently from 4fd1a57 to 1fb9b9b Compare June 24, 2024 08:24
@seb-jean
Copy link
Contributor

Very useful component :)

@Kocal Kocal force-pushed the feat/38-map-component branch 4 times, most recently from c97965e to f73bf19 Compare June 24, 2024 11:47
src/Map/assets/package.json Outdated Show resolved Hide resolved
@simondaigre
Copy link
Contributor

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

@Kocal
Copy link
Collaborator Author

Kocal commented Jun 24, 2024

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 :)

@smnandre
Copy link
Collaborator

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

@smnandre
Copy link
Collaborator

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) ?

@Kocal
Copy link
Collaborator Author

Kocal commented Jun 27, 2024

... 😮‍💨 , 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?

src/Map/.gitignore Outdated Show resolved Hide resolved
src/Map/LICENSE Outdated Show resolved Hide resolved
src/Map/README.md Outdated Show resolved Hide resolved
src/Map/composer.json Outdated Show resolved Hide resolved
src/Map/composer.json Outdated Show resolved Hide resolved
src/Map/composer.json Outdated Show resolved Hide resolved
@Kocal Kocal requested a review from smnandre August 6, 2024 14:32
@javiereguiluz
Copy link
Member

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 javiereguiluz merged commit 4a65595 into symfony:2.x Aug 7, 2024
39 checks passed
@Kocal Kocal deleted the feat/38-map-component branch August 7, 2024 13:01
@Kocal
Copy link
Collaborator Author

Kocal commented Aug 7, 2024

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!

@fabpot
Copy link
Member

fabpot commented Aug 7, 2024

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

@Kocal
Copy link
Collaborator Author

Kocal commented Aug 7, 2024

So quick! :D

We will need dedicated repositories for:

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 :)

@@ -0,0 +1,33 @@
{
"name": "symfony/ux-map-google",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be symfony/ux-google-map to be consistent with how we are naming bridges in Symfony.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #2031

@@ -0,0 +1,33 @@
{
"name": "symfony/ux-map-leaflet",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be symfony/ux-leaflet-map to be consistent with how we are naming bridges in Symfony.

javiereguiluz added a commit that referenced this pull request Aug 7, 2024
…Kocal)

This PR was merged into the 2.x branch.

Discussion
----------

[Map] Rename "symfony/ux-map-%s" to "symfony/ux-%s-map"

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - For new features, provide some code snippets to help understand usage.
 - Features and deprecations must be submitted against branch main.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

Following #1937 (review).

Commits
-------

8e10049 [Map] Rename "symfony/ux-map-%s" to "symfony/ux-%s-map"
@fabpot
Copy link
Member

fabpot commented Aug 7, 2024

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

@Kocal
Copy link
Collaborator Author

Kocal commented Aug 7, 2024

@fabpot thanks!
Except for the folder src/Bridge that should not be present in symfony/ux-map, everything is perfect!

@smnandre
Copy link
Collaborator

smnandre commented Aug 7, 2024

Cannot wait to play with it!

What an incredible PR this was, thank you very much for your patience, hard work and positive spirit @Kocal!

👏

@fabpot
Copy link
Member

fabpot commented Aug 7, 2024

@fabpot thanks! Except for the folder src/Bridge that should not be present in symfony/ux-map, everything is perfect!

Good catch, I used Bridge like for Symfony, the src/xxx/src/yyy disturbed me :)
BUT, excluding a deep directory is NOT supported by the splitter currently. I don't remember why I put this restriction in place, so I need to double-check that.

@smnandre
Copy link
Collaborator

smnandre commented Aug 7, 2024

We can move folders around in the ux mono if that can ease things.

@fabpot
Copy link
Member

fabpot commented Aug 7, 2024

All good now 🤞

javiereguiluz added a commit that referenced this pull request Aug 8, 2024
This PR was merged into the 2.x branch.

Discussion
----------

[Map][Leaflet] Fix Marker inside-circle

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - For new features, provide some code snippets to help understand usage.
 - Features and deprecations must be submitted against branch main.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

With Leaflet, the original marker's icon (that you can find at https://www.jsdelivr.com/package/npm/leaflet?tab=files&path=dist%2Fimages), has a white circle in it, [see example](https://leafletjs.com/index.html):
<img width="908" alt="Capture d’écran 2024-08-08 à 14 44 03" src="https://github.com/user-attachments/assets/21afcff7-6c23-40e8-865b-2f54700830b0">

In #1937, in my first iteration I've added Leaflet icons to Symfony AssetMapper/ImportMap but this was toooo much work.
So, with Simon, we have agreed to use an inlined SVG instead, I've used the one from https://www.jsdelivr.com/package/npm/leaflet?tab=files&path=src%2Fimages, but it looks like the circle is transparent instead of white:
<img width="588" alt="Capture d’écran 2024-08-08 à 14 39 05" src="https://github.com/user-attachments/assets/eea48349-57c4-4caf-b25d-7793c6b282f1">

With this PR:
<img width="570" alt="Capture d’écran 2024-08-08 à 14 44 37" src="https://github.com/user-attachments/assets/3832aca7-7bb4-45c7-a0e4-1c58c7a8da0c">

Commits
-------

06e144b [Map][Leaflet] Fix Marker inside circle
@Kocal
Copy link
Collaborator Author

Kocal commented Aug 8, 2024

Thanks Fabien :)

kbond added a commit that referenced this pull request Aug 12, 2024
…ter if fit bounds to markers (Kocal)

This PR was merged into the 2.x branch.

Discussion
----------

[Map] Add the possibility to not configure map zoom/center if fit bounds to markers

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - For new features, provide some code snippets to help understand usage.
 - Features and deprecations must be submitted against branch main.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

Following a review from `@smnandre` in #1937 or in our Slack DMs.

Forcing the developper to explicitly configure the center/zoom of the map does not make sense if `fitBoundsToMarker` feature is enabled.

Also, I prefer throw exceptions instead of automatically enable `fitBoundsToMarker` if there are markers and center/zoom are not configured, it's more explicit like that and it mimic the Leaflet/GoogleMaps behaviors (the map can't be rendered if no center/zoom, and `fitBoundsToMarker` is a UX Map's feature).

Commits
-------

b3e6412 [Map] Add the possibility to not configure map zoom/center if fit bounds to markers
kbond added a commit that referenced this pull request Aug 13, 2024
…kage.json` resolving (Kocal)

This PR was merged into the 2.x branch.

Discussion
----------

[Map] Re-add keyword "symfony-ux", to fix Symfony Flex `package.json` resolving

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - For new features, provide some code snippets to help understand usage.
 - Features and deprecations must be submitted against branch main.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

In #1937 I was asked to remove the keyword `symfony-ux` in favor `symfony` and `ux`, but it looks like it break Symfony Flex behaviour to resolve the package's `package.json`: https://github.com/symfony/flex/blob/2.x/src/PackageJsonSynchronizer.php#L372

No `importmap.php` nor `assets/controllers.json` were updated when installing UX Map bridges :(

I'm adding back this keyword and removing the other ones. Sorry for the inconvenience 🙏

Commits
-------

e417b4b [Map] Re-add keyword "symfony-ux", to fix Symfony Flex behaviour (importmap, controllers...)
@Kocal Kocal added the Map label Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Map Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: map component
8 participants