Skip to content

Commit

Permalink
feature #2045 [Map] Add the possibility to not configure map zoom/cen…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
kbond committed Aug 12, 2024
2 parents 21595a7 + b3e6412 commit c3e42ab
Show file tree
Hide file tree
Showing 10 changed files with 56 additions and 30 deletions.
8 changes: 4 additions & 4 deletions src/Map/assets/dist/abstract_map_controller.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ export type Point = {
lng: number;
};
export type MapView<Options, MarkerOptions, InfoWindowOptions> = {
center: Point;
zoom: number;
center: Point | null;
zoom: number | null;
fitBoundsToMarkers: boolean;
markers: Array<MarkerDefinition<MarkerOptions, InfoWindowOptions>>;
options: Options;
Expand Down Expand Up @@ -38,8 +38,8 @@ export default abstract class<MapOptions, Map, MarkerOptions, Marker, InfoWindow
initialize(): void;
connect(): void;
protected abstract doCreateMap({ center, zoom, options, }: {
center: Point;
zoom: number;
center: Point | null;
zoom: number | null;
options: MapOptions;
}): Map;
createMarker(definition: MarkerDefinition<MarkerOptions, InfoWindowOptions>): Marker;
Expand Down
8 changes: 4 additions & 4 deletions src/Map/assets/src/abstract_map_controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import { Controller } from '@hotwired/stimulus';
export type Point = { lat: number; lng: number };

export type MapView<Options, MarkerOptions, InfoWindowOptions> = {
center: Point;
zoom: number;
center: Point | null;
zoom: number | null;
fitBoundsToMarkers: boolean;
markers: Array<MarkerDefinition<MarkerOptions, InfoWindowOptions>>;
options: Options;
Expand Down Expand Up @@ -93,8 +93,8 @@ export default abstract class<
zoom,
options,
}: {
center: Point;
zoom: number;
center: Point | null;
zoom: number | null;
options: MapOptions;
}): Map;

Expand Down
3 changes: 3 additions & 0 deletions src/Map/doc/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,11 @@ A map is created by calling ``new Map()``. You can configure the center, zoom, a
{
// 1. Create a new map instance
$myMap = (new Map());
// Explicitly set the center and zoom
->center(new Point(46.903354, 1.888334))
->zoom(6)
// Or automatically fit the bounds to the markers
->fitBoundsToMarkers()
;
// 2. You can add markers
Expand Down
4 changes: 2 additions & 2 deletions src/Map/src/Bridge/Google/assets/dist/map_controller.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ export default class extends AbstractMapController<MapOptions, google.maps.Map,
providerOptionsValue: Pick<LoaderOptions, 'apiKey' | 'id' | 'language' | 'region' | 'nonce' | 'retries' | 'url' | 'version'>;
connect(): Promise<void>;
protected doCreateMap({ center, zoom, options, }: {
center: Point;
zoom: number;
center: Point | null;
zoom: number | null;
options: MapOptions;
}): google.maps.Map;
protected doCreateMarker(definition: MarkerDefinition<google.maps.marker.AdvancedMarkerElementOptions, google.maps.InfoWindowOptions>): google.maps.marker.AdvancedMarkerElement;
Expand Down
4 changes: 2 additions & 2 deletions src/Map/src/Bridge/Google/assets/src/map_controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ export default class extends AbstractMapController<
zoom,
options,
}: {
center: Point;
zoom: number;
center: Point | null;
zoom: number | null;
options: MapOptions;
}): google.maps.Map {
// We assume the following control options are enabled if their options are set
Expand Down
6 changes: 3 additions & 3 deletions src/Map/src/Bridge/Leaflet/assets/dist/map_controller.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ type MapOptions = Pick<LeafletMapOptions, 'center' | 'zoom'> & {
};
export default class extends AbstractMapController<MapOptions, typeof LeafletMap, MarkerOptions, Marker, Popup, PopupOptions> {
connect(): void;
protected doCreateMap({ center, zoom, options }: {
center: Point;
zoom: number;
protected doCreateMap({ center, zoom, options, }: {
center: Point | null;
zoom: number | null;
options: MapOptions;
}): LeafletMap;
protected doCreateMarker(definition: MarkerDefinition): Marker;
Expand Down
6 changes: 3 additions & 3 deletions src/Map/src/Bridge/Leaflet/assets/dist/map_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ class map_controller extends AbstractMapController {
});
super.connect();
}
doCreateMap({ center, zoom, options }) {
doCreateMap({ center, zoom, options, }) {
const map$1 = map(this.element, {
...options,
center,
zoom,
center: center === null ? undefined : center,
zoom: zoom === null ? undefined : zoom,
});
tileLayer(options.tileLayer.url, {
attribution: options.tileLayer.attribution,
Expand Down
10 changes: 7 additions & 3 deletions src/Map/src/Bridge/Leaflet/assets/src/map_controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,15 @@ export default class extends AbstractMapController<
super.connect();
}

protected doCreateMap({ center, zoom, options }: { center: Point; zoom: number; options: MapOptions }): LeafletMap {
protected doCreateMap({
center,
zoom,
options,
}: { center: Point | null; zoom: number | null; options: MapOptions }): LeafletMap {
const map = createMap(this.element, {
...options,
center,
zoom,
center: center === null ? undefined : center,
zoom: zoom === null ? undefined : zoom,
});

createTileLayer(options.tileLayer.url, {
Expand Down
16 changes: 9 additions & 7 deletions src/Map/src/Map.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,16 +85,18 @@ public function addMarker(Marker $marker): self

public function toArray(): array
{
if (null === $this->center) {
throw new InvalidArgumentException('The center of the map must be set.');
}

if (null === $this->zoom) {
throw new InvalidArgumentException('The zoom of the map must be set.');
if (!$this->fitBoundsToMarkers) {
if (null === $this->center) {
throw new InvalidArgumentException('The map "center" must be explicitly set when not enabling "fitBoundsToMarkers" feature.');
}

if (null === $this->zoom) {
throw new InvalidArgumentException('The map "zoom" must be explicitly set when not enabling "fitBoundsToMarkers" feature.');
}
}

return [
'center' => $this->center->toArray(),
'center' => $this->center?->toArray(),
'zoom' => $this->zoom,
'fitBoundsToMarkers' => $this->fitBoundsToMarkers,
'options' => (object) ($this->options?->toArray() ?? []),
Expand Down
21 changes: 19 additions & 2 deletions src/Map/tests/MapTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class MapTest extends TestCase
public function testCenterValidation(): void
{
self::expectException(InvalidArgumentException::class);
self::expectExceptionMessage('The center of the map must be set.');
self::expectExceptionMessage('The map "center" must be explicitly set when not enabling "fitBoundsToMarkers" feature.');

$map = new Map();
$map->toArray();
Expand All @@ -33,14 +33,31 @@ public function testCenterValidation(): void
public function testZoomValidation(): void
{
self::expectException(InvalidArgumentException::class);
self::expectExceptionMessage('The zoom of the map must be set.');
self::expectExceptionMessage('The map "zoom" must be explicitly set when not enabling "fitBoundsToMarkers" feature.');

$map = new Map(
center: new Point(48.8566, 2.3522)
);
$map->toArray();
}

public function testZoomAndCenterCanBeOmittedIfFitBoundsToMarkers(): void
{
$map = new Map(
fitBoundsToMarkers: true
);

$array = $map->toArray();

self::assertSame([
'center' => null,
'zoom' => null,
'fitBoundsToMarkers' => true,
'options' => $array['options'],
'markers' => [],
], $array);
}

public function testWithMinimumConfiguration(): void
{
$map = new Map();
Expand Down

0 comments on commit c3e42ab

Please sign in to comment.