Skip to content

feat: add ContainCameraLatitude#2073

Merged
JaffaKetchup merged 3 commits intofleaflet:masterfrom
mbenoukaiss:feature-2071
Apr 30, 2025
Merged

feat: add ContainCameraLatitude#2073
JaffaKetchup merged 3 commits intofleaflet:masterfrom
mbenoukaiss:feature-2071

Conversation

@mbenoukaiss
Copy link
Copy Markdown
Contributor

@mbenoukaiss mbenoukaiss commented Apr 9, 2025

Allows constraining the camera vertically and not horizontally #2071

  • ContainCameraVertically or ContainCameraLatitudinally ?
  • I'm not quite happy with the a and b parameters naming, but I don't like anything that would mention a notion of order (like min and max) because it does not quite make sense, let my know if this is alright or if you have a suggestion
  • You mentioned "this could be made the new default (maybe only if Crs.replicateWorldLongitude is true)", even though I suppose not many people, if any, use the default CameraConstraint.unconstrained(), it would potentially be a breaking change. Also not 100% familiar with dart but I could not find a way to keep the constructor constant with a condition on crs.replicatesWorldLongitude.

Copy link
Copy Markdown
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

Thanks @mbenoukaiss! Sorry I haven't got to this earlier.

We do need to find some way to describe a & b. I can live with the names (although they're not ideal), but we do need to put a docstring on each of them. I know you said you'd prefer not to imply order, but I'm not sure why? Would positive & negative or north and south work?

I'm assuming you set a & b to the latitudes, such as 180 and -180? Maybe it's a good idea to add this to one of the examples, but I can find a space for that.

I think ContainCameraLatitude might be a better name, as it doesn't matter about rotation.

On that note, I need to test this to make sure it works correctly when the map is rotated as well. Would it be possible to add some unit tests for this as well?

Also not 100% familiar with dart but I could not find a way to keep the constructor constant with a condition on crs.replicatesWorldLongitude.

That's a good point. I'll think about it, but I think this is probably OK for now.

Added defaults of 90 & -90
Added documentation
Added to example page
@JaffaKetchup JaffaKetchup changed the title feat: add ContainCameraVertically feat: add ContainCameraLatitude Apr 30, 2025
@JaffaKetchup JaffaKetchup enabled auto-merge (squash) April 30, 2025 17:23
Copy link
Copy Markdown
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

LGTM, thanks :). Seems to work with rotation.

@JaffaKetchup JaffaKetchup merged commit c428d6e into fleaflet:master Apr 30, 2025
7 checks passed
@mbenoukaiss
Copy link
Copy Markdown
Contributor Author

Hey, sorry I haven't had much time to make the changes. You're right about north/south, it makes more sense but I did not think of it, I was thinking about top/bottom which did not make sense because of map rotation. ContainCameraLatitude is indeed a better choice!

@JaffaKetchup JaffaKetchup linked an issue May 15, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Constraining only latitude with unconstrained horizontal scroll

2 participants