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] markers, polygons and polylines removal #2547

Merged
merged 1 commit into from
Feb 15, 2025

Conversation

sblondeau
Copy link
Contributor

@sblondeau sblondeau commented Feb 2, 2025

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

Add new methods to remove markers, polygons or polylines (map elements)
Map elements will be store in SplObjectStorage instead of arrays.

When create a map element, you can now add an optional identfier (string).
New methods getMarker(string identifier), getPolygon(string identifier) and getPolyline(string $identifier) car retreive a map element from its identfiier.

example
$departureMarker = new Marker (
position: new Point(45.7640, 4.8357),
title: 'Lyon',
identifier: 'departure'
)

$map->addMarker($departureMarker);

// remove marker with
$map->removeMarker($departureMarker)
// or 
$map->removeMarker($map->getMarker('departure'));

@sblondeau sblondeau requested a review from Kocal as a code owner February 2, 2025 15:03
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed Feature New Feature Map labels Feb 2, 2025
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.

Hi @sblondeau and thanks for your suggestion, I was waiting for someone to open it! :D

It's a very, very good start, but there are still things to refactor and simplify:

  1. I believe Map::remove*() methods must accept Marker|string $markerOrId as parameter, so we can remove methods Map::get*()
  2. Map::get*() methods do not really make sense to me, since there is no modifiers on Marker/Polygon/Polyline. Once you construct them, you can't alter them, so not point for Map::get*().
  3. Some architecture things can be improved, see review comments
  4. Types definitions in assets/ must be updated to, since you added a new property identifier (which can be shortened and changed to id)
  5. Also, please apply Fapbot's suggestions

Thanks, and do not hesitate if necessary! :)

@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Feb 2, 2025
@smnandre
Copy link
Member

smnandre commented Feb 8, 2025

Do you need some help here @sblondeau ?

@sblondeau
Copy link
Contributor Author

Do you need some help here @sblondeau ?

No time at all last week, but I will try to work on it in the next day :-) Thanks for your comments

@sblondeau
Copy link
Contributor Author

2. `Map::get*()` methods do not really make sense to me, since there is no modifiers on `Marker`/`Polygon`/`Polyline`. Once you construct them, you can't alter them, so not point for `Map::get*()`.

Thanks for your comments @Kocal. One question, that's true that a Marker is readonly... but can we imagine that we getMarker() to modify its position, title or infoWindow ? so we should remove and recreate it ?

@Kocal
Copy link
Member

Kocal commented Feb 8, 2025

2. `Map::get*()` methods do not really make sense to me, since there is no modifiers on `Marker`/`Polygon`/`Polyline`. Once you construct them, you can't alter them, so not point for `Map::get*()`.

Thanks for your comments @Kocal. One question, that's true that a Marker is readonly... but can we imagine that we getMarker() to modify its position, title or infoWindow ? so we should remove and recreate it ?

For the moment, yes, we can implement Marker, Polygon and Polyline removal without the need of Map::get*().

If really necessary, let's open a separate PR to make them non-readonly and implement Map::get*().

@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Feb 10, 2025
@sblondeau sblondeau requested review from Kocal and smnandre February 10, 2025 22:26
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 applying suggestions.

Can you please revert changes unrelated to Map and the removal feature please? 🙏🏻

@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Feb 11, 2025
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Feb 12, 2025
@sblondeau sblondeau force-pushed the feat/removeMarker branch 2 times, most recently from 277d6a6 to 3acd83e Compare February 12, 2025 21:21
@sblondeau sblondeau requested a review from Kocal February 12, 2025 21:29
@sblondeau sblondeau force-pushed the feat/removeMarker branch 3 times, most recently from 51ff6a5 to a27bd08 Compare February 13, 2025 21:23
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.

I really like where we are now!

I'll test your PR locally before approving it and merging it.

@Kocal
Copy link
Member

Kocal commented Feb 15, 2025

I've tested your PR and it works well:

Enregistrement.de.l.ecran.2025-02-15.a.17.14.57.mov

I did some minor adjustments to mark those classes as internal.

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Feb 15, 2025
@Kocal Kocal force-pushed the feat/removeMarker branch from 7f48247 to e19fff9 Compare February 15, 2025 16:19
@Kocal
Copy link
Member

Kocal commented Feb 15, 2025

Thanks @sblondeau.

Kocal added a commit that referenced this pull request Feb 15, 2025
…u, Kocal)

This PR was merged into the 2.x branch.

Discussion
----------

[Map] markers, polygons and polylines removal

| 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

Add new methods to remove markers, polygons or polylines (map elements)
Map elements will be store in SplObjectStorage instead of arrays.

When create a map element, you can now add an optional identfier (string).
New methods `getMarker(string identifier)`, `getPolygon(string identifier)` and `getPolyline(string $identifier)` car retreive a map element from its identfiier.

example
    $departureMarker = new Marker (
        position: new Point(45.7640, 4.8357),
        title: 'Lyon',
        identifier: 'departure'
    )

    $map->addMarker($departureMarker);

    // remove marker with
    $map->removeMarker($departureMarker)
    // or
    $map->removeMarker($map->getMarker('departure'));

Commits
-------

7f48247 [Map] Minor adjustements
a27bd08 allow marker, polygone and polyline removal
@Kocal Kocal merged commit 5487937 into symfony:2.x Feb 15, 2025
56 of 60 checks passed
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.

4 participants