Skip to content

Conversation

@durgax04
Copy link

@durgax04 durgax04 commented Oct 24, 2025

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Hi @nemesifier

This PR improves how map options are initialized in LeafletCoordSys.create.
It now creates a new object starting with a default { worldCopyJump: true } and then applies or overrides any user-provided settings.
This helps ensure better user experience when panning across ±180° longitude (antimeridian) by enabling worldCopyJump by default.

worldCopyJump.webm

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Could you add a simple test which asserts that this worldCopyJump option is set to true?

This will protect us from the possibility that someone removes this by mistake in the future.

@durgax04
Copy link
Author

durgax04 commented Oct 25, 2025

Thanks for the review! The tests are now passing locally after adjusting the map options and cleaning up the formatting.
Please let me know if any further changes are needed.

Screenshot from 2025-10-29 11-59-11

@durgax04
Copy link
Author

durgax04 commented Oct 29, 2025

While running all tests, a couple of existing browser tests failed — for example:

  • Chart Rendering Test › render floorplan map without console errors
  • Chart Rendering Test › render wifi clients example without errors
    These failures seem unrelated to my change, since they also occur when my new test file is removed.

My new test passes successfully and confirms that worldCopyJump is true by default.

Could you please confirm if these browser tests are known issues, or should I look into them further?

@nemesifier nemesifier changed the title fix: add default map option (worldCopyJump: true) to LeafletCoordSys [fix] Added default leaflet map option worldCopyJump: true to LeafletCoordSys Nov 7, 2025
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

LGTM, I'll just rename the test file for consistency.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Upon closer inspection, I see there's a test which is failing for real (not a false positive).

@nemesifier
Copy link
Member

I am leaning towards a simpler approach, see #466.

Thanks for contributing!

@nemesifier nemesifier closed this Nov 7, 2025
@durgax04
Copy link
Author

durgax04 commented Nov 7, 2025

Thanks for the feedback!
I’ll take a look at #466 to see the simpler approach.
Glad I could help confirm the issue and contribute to the discussion.
Appreciate your time and guidance!

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.

2 participants