Skip to content

[Map] Make UX Map compatible with Live Components (and some internal things) #2385

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 22, 2024
Merged

[Map] Make UX Map compatible with Live Components (and some internal things) #2385

merged 5 commits into from
Nov 22, 2024

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented Nov 17, 2024 •
edited
Loading

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

Here’s a corrected and polished version of your text:

Hi! 😊

This PR enhances the UX Map to be compatible with Live Components, allowing you to interact with the Map directly from your PHP code. You can perform actions and see your map update in real-time from the front-end!

To achieve this, I had to refactor and improve several areas:

  1. Due to the hydration and de-hydration processes of Live Components, I ensured that the toArray and new fromArray methods for Map, Marker, and all other value objects could rebuild an equivalent object accurately (e.g., $marker == Marker::fromArray($marker->toArray())).
  2. Since Stimulus monitors value changes, it wasn’t efficient to store everything in a single large view object containing zoom, center, markers, etc. These properties were split into individual values to improve performance.
  3. Before rendering the map, an @id is now automatically computed for each Marker and Polygon definition. This optimization makes rendering significantly more efficient, avoiding the need to remove and re-render all markers or polygons (and avoiding a visual glitch aswell).
Enregistrement.de.l.ecran.2024-11-17.a.10.30.54-2.mov
Enregistrement.de.l.ecran.2024-11-17.a.10.28.15-2.mov

@carsonbot carsonbot added Feature New Feature Map Status: Needs Review Needs to be reviewed labels Nov 17, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 17, 2024 •
edited
Loading

📊 Packages dist files size difference

Thanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
Please review the changes and make sure they are expected.

FileBefore (Size / Gzip)After (Size / Gzip)
Map
abstract_map_controller.d.ts 3 kB / 741 B 3.31 kB+10% 📈 / 821 B+11% 📈
abstract_map_controller.js 1.88 kB / 610 B 3.23 kB+72% 📈 / 843 B+38% 📈
Map (Bridge Google)
map_controller.d.ts 2.07 kB / 729 B 2.19 kB+6% 📈 / 733 B+1% 📈
map_controller.js 7.07 kB / 1.67 kB 8.7 kB+23% 📈 / 1.95 kB+17% 📈
Map (Bridge Leaflet)
map_controller.d.ts 1.31 kB / 544 B 1.57 kB+20% 📈 / 579 B+6% 📈
map_controller.js 5.52 kB / 1.77 kB 7.2 kB+30% 📈 / 2.06 kB+16% 📈

@Kocal Kocal force-pushed the imp-map-split-view-attrs branch 6 times, most recently from 00fc2d4 to bcbc42a Compare November 17, 2024 10:17
@Kocal Kocal requested review from kbond, smnandre and WebMamba November 17, 2024 10:18
@Kocal
Copy link
Member Author

Kocal commented Nov 17, 2024

Note: Usages and examples can be a bit limited for the moment, but for a next PR I would like to add more methods like getZoom() or removeMarker() (...)

smnandre
smnandre reviewed Nov 19, 2024
View reviewed changes
@Kocal Kocal force-pushed the imp-map-split-view-attrs branch from bcbc42a to 635bf7c Compare November 19, 2024 19:48
@Kocal Kocal mentioned this pull request Nov 19, 2024
Merged
@Kocal Kocal requested a review from smnandre November 20, 2024 20:29
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
src/Map/src/Live/ComponentWithMapTrait.php
/**
* @internal
*/
#[LiveProp(hydrateWith: 'hydrateMap', dehydrateWith: 'dehydrateMap')]
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
#[LiveProp(hydrateWith: 'hydrateMap', dehydrateWith: 'dehydrateMap')]
#[LiveProp(hydrateWith: 'hydrateMap', dehydrateWith: 'dehydrateMap')]

Should be private i think, and we maybe should create a dedicated hydration extension here.. wdyt ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I put it public otherwise the Live hydration process was not working due to visibility issue. Maybe I can try to add a setter?

Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss all this tomorrow :)

smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 20, 2024
View reviewed changes
@Kocal Kocal force-pushed the imp-map-split-view-attrs branch 2 times, most recently from 074fa84 to c90ac6d Compare November 20, 2024 23:27
smnandre
smnandre reviewed Nov 21, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 21, 2024
View reviewed changes
smnandre
smnandre reviewed Nov 21, 2024
View reviewed changes
smnandre
smnandre approved these changes Nov 21, 2024
View reviewed changes
Copy link
Member

@smnandre smnandre left a comment

Choose a reason for hiding this comment

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

Nice!!

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Nov 21, 2024
@Kocal Kocal force-pushed the imp-map-split-view-attrs branch from c90ac6d to 558eb5c Compare November 21, 2024 08:15
smnandre
smnandre reviewed Nov 22, 2024
View reviewed changes
src/Map/src/Live/ComponentWithMapTrait.php Outdated
Comment on lines 21 to 26
/**
* @author Hugo Alliaume <hugo@alliau.me>
*/
trait ComponentWithMapTrait
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
/**
* @author Hugo Alliaume <hugo@alliau.me>
*/
trait ComponentWithMapTrait
/**
* @author Hugo Alliaume <hugo@alliau.me>
*
* @experimental
*/
trait ComponentWithMapTrait

@Kocal Kocal force-pushed the imp-map-split-view-attrs branch from 558eb5c to 6d4ead0 Compare November 22, 2024 21:25
@Kocal Kocal force-pushed the imp-map-split-view-attrs branch from 6d4ead0 to 74d937a Compare November 22, 2024 21:26
@Kocal Kocal merged commit 7bef0bf into symfony:2.x Nov 22, 2024
61 checks passed
@Kocal Kocal deleted the imp-map-split-view-attrs branch November 22, 2024 21:43
@Kocal Kocal mentioned this pull request Dec 2, 2024
Merged
Kocal added a commit that referenced this pull request Dec 3, 2024
@Kocal
bug #2420 [Map] Fix default values of Stimulus Map Controller (Kocal)
56cef50 
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Map] Fix default values of Stimulus Map Controller

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Fix #2417 <!-- 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).
-->

This issue happens at `doCreateMap` when we called `->fitBoundsToMarkers()` instead of `->center(...)`.

The value `center` was supposed to be `null`, but since I forgot to configure the default value of `this.centerValue` to `null`, it automatically used `{}` as default value (https://stimulus.hotwired.dev/reference/values#getters) and it breaks the code.

This bug has been introduced in #2385.

Commits
-------

4cb91ef [Map] Fix default values of Stimulus Map Controller
@Kocal Kocal mentioned this pull request Feb 8, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smnandre smnandre smnandre approved these changes

@kbond kbond Awaiting requested review from kbond

@WebMamba WebMamba Awaiting requested review from WebMamba

Assignees
No one assigned
Labels
Feature New Feature Map Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Kocal @smnandre @carsonbot