Skip to content

Conversation

smnandre
Copy link
Member

@smnandre smnandre commented Sep 1, 2024

Q A
Bug fix? no
New feature? yes
Issues Fix #...
License MIT
  • add an MapFactory (internal)
  • allow ux_map Twig function to render map from named arguments
  • add basic TwigComponent <twig:ux:map ... />

Create Map from Twig

{{ ux_map(
    center: [51.5074, 0.1278],
    zoom: 3,
    markers:  [
        { position: [51.5074, 0.1278], title: 'London' },
        { position: [48.8566, 2.3522], title: 'Paris' },
        {
            position: [40.7128, -74.0060],
            title: 'New York',
            infoWindow: { content: 'Welcom to <b>New York</b>' }
        },
    ],
    attributes: {
        class: 'foo',
        style: 'height: 800px; width: 100%; border: 4px solid red; margin-block: 10vh;',
    }
) }}

ux-map

@carsonbot carsonbot added Feature New Feature Map Status: Needs Review Needs to be reviewed labels Sep 1, 2024
@smnandre smnandre requested a review from Kocal September 1, 2024 07:30
Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!
It looks nice to me, but I have many comments on the validation part where I'm not 100% sure of its legitimacy (we use PHP types, and I'm afraid all those code/function calls can downgrade performances)

@Kocal
Copy link
Member

Kocal commented Sep 11, 2024

See #2152 as a lighter (but not complete yet) alternative

@smnandre
Copy link
Member Author

Yeah i'll use the fromArray static from your PR and we'll see... but i hope this will simplify a lot this one.

Thanks !

@smnandre smnandre force-pushed the map/twig-function-component branch from 606594b to 553e49b Compare September 11, 2024 22:42
@smnandre
Copy link
Member Author

Integrated the ideas fom #2152

@smnandre smnandre changed the title [Map] Enhance ux_map() + Add <twig:ux:map/> TwigComponent [Map] Render map from Twig with ux_map() and <twig:ux:map /> Sep 12, 2024
Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

That looks much better to me, thanks :)

Still minor comments tho

*/
public static function fromArray(array $point): self
{
if (isset($point['lat'], $point['lng'])) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (isset($point['lat'], $point['lng'])) {
if ($point['lat'] ?? null && $point['lng'] ?? null) {

so we can allow users to pass 0 as lat/lng

Copy link
Member Author

Choose a reason for hiding this comment

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

$pos = ['lat' => 0, 'lng' => 0];

echo isset($pos['lat'], $pos['lng']);

# 1

The only case would be $pos = [0, 0] and i don't mind that much 😅

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Sep 12, 2024
Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

(sorry I approved by mistake)

use Symfony\UX\TwigComponent\TwigComponentBundle;

/**
* @internal
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to mark test files internal?

@Kocal Kocal force-pushed the map/twig-function-component branch from 4422a2f to 92f89d3 Compare September 18, 2024 08:16
@Kocal
Copy link
Member

Kocal commented Sep 18, 2024

Thanks @smnandre for working on this feature, this is much appreciated.

@Kocal Kocal merged commit c05aff1 into symfony:2.x Sep 18, 2024
9 checks passed
@Kocal Kocal deleted the map/twig-function-component branch September 18, 2024 08:26
@smnandre
Copy link
Member Author

Thanks @Kocal for the help / suggestions / feedback ❤️

I'll add doc asap, but I now can work on demos with twig content, and i think it's gonna be more effective to showcase the component!

smnandre added a commit that referenced this pull request Nov 4, 2024
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Map] Update documentation

First round of changes, very open to feedback / suggestions
(it's a draft to start iterating)

related: #2117

Commits
-------

9eb8539 [Map] Update documentation
symfony-splitter pushed a commit to symfony/ux-map that referenced this pull request Nov 4, 2024
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Map] Update documentation

First round of changes, very open to feedback / suggestions
(it's a draft to start iterating)

related: symfony/ux#2117

Commits
-------

9eb85399a [Map] Update documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Map Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants