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.

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

[Map] Add the possibility to not configure map zoom/center if fit bounds to markers #2045

Merged
merged 1 commit into from
Aug 12, 2024
Merged

[Map] Add the possibility to not configure map zoom/center if fit bounds to markers #2045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Map/assets/dist/abstract_map_controller.d.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.');
Copy link
Member

Choose a reason for hiding this comment

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

I still think we should make it true per default 🙊

Copy link
Member Author

Choose a reason for hiding this comment

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

😠

Copy link
Member Author

@Kocal Kocal Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

More seriously, fit bounds to marker is not a feature from Google Maps or Leaflet, it's a feature from UX Map, and enable it automatically depending of certains conditions looks wrong to me, because it behaves differently than GMaps/Leaflet in this situation.

If we make it true by default, we should disable it (if not explicitly enabled) if the dev provide a center / zoom configuration.

But, that's true it can offers a better DX, because actually if you create a map (without configuring the zoom / center, fit bounds to marker), nothing is displayed (either grey map, or errors in console).
If we enable it by default (and disable it if necessary), the user will see its map without any necessary configuration


$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