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] Add the possibility to not configure map zoom/center if fit bounds to markers #2045

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

Kocal
Copy link
Collaborator

@Kocal Kocal commented Aug 11, 2024

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

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).

@carsonbot carsonbot added Feature New Feature Status: Needs Review Needs to be reviewed labels Aug 11, 2024
@Kocal Kocal force-pushed the feat/ux-map-no-errors-if-fit-bounds branch 2 times, most recently from 3acffb5 to 9914af6 Compare August 11, 2024 06:05
@Kocal Kocal force-pushed the feat/ux-map-no-errors-if-fit-bounds branch from 9914af6 to b3e6412 Compare August 11, 2024 06:07
@Kocal Kocal requested review from kbond, chalasr, WebMamba and smnandre and removed request for chalasr August 11, 2024 06:09
@@ -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
Collaborator

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
Collaborator Author

Choose a reason for hiding this comment

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

😠

Copy link
Collaborator Author

@Kocal Kocal Aug 12, 2024

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

@kbond
Copy link
Member

kbond commented Aug 12, 2024

Thanks Hugo.

@kbond kbond merged commit c3e42ab into symfony:2.x Aug 12, 2024
39 checks passed
@Kocal Kocal added the Map label Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Map Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants