-
-
Notifications
You must be signed in to change notification settings - Fork 357
[Map] Render map from Twig with ux_map()
and <twig:ux:map />
#2117
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
Conversation
There was a problem hiding this 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)
See #2152 as a lighter (but not complete yet) alternative |
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 ! |
606594b
to
553e49b
Compare
Integrated the ideas fom #2152 |
ux_map()
+ Add <twig:ux:map/>
TwigComponentux_map()
and <twig:ux:map />
There was a problem hiding this 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'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (isset($point['lat'], $point['lng'])) { | |
if ($point['lat'] ?? null && $point['lng'] ?? null) { |
so we can allow users to pass 0
as lat/lng
There was a problem hiding this comment.
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 😅
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
4422a2f
to
92f89d3
Compare
Thanks @smnandre for working on this feature, this is much appreciated. |
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! |
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
add an MapFactory (internal)ux_map
Twig function to render map from named arguments<twig:ux:map ... />
Create Map from Twig