Migrate Kartographer to Leaflet 1.x
Closed, ResolvedPublic

Description

When Mapbox JS client lib supports it, we should migrate to leaflet 1.x.

This is different from T147361, which is about updating the Kartotherian service's static resources.

Yurik created this task.Oct 4 2016, 7:20 PM
Restricted Application added a project: Discovery. · View Herald TranscriptOct 4 2016, 7:20 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Yurik renamed this task from Migrate to Leaflet 1.x to Migrate Kartographer to Leaflet 1.x.Oct 6 2016, 5:43 PM
Yurik updated the task description. (Show Details)
Yurik moved this task from Unsorted to UI tasks on the Maps (Kartographer) board.Oct 6 2016, 8:31 PM

Change 317546 had a related patch set uploaded (by JGirault):
WIP: Upgrade mapbox.js to v3.0 (upgrade leaflet to 1.0)

https://gerrit.wikimedia.org/r/317546

Yurik removed a project: Maps.Dec 15 2016, 4:39 AM
JGirault moved this task from Backlog to Needs review on the Maps-Sprint board.Mar 17 2017, 10:40 PM

@MaxSem any idea how to fix the failing phpunit tests?

https://integration.wikimedia.org/ci/job/mwext-testextension-hhvm-jessie/557/console

22:07:51 There was 1 failure:
22:07:51 
22:07:51 1) ResourcesTest::testFileExistence with data set #602 ('/home/jenkins/workspace/mwext...lt#VML', 'mapbox', '/home/jenkins/workspace/mwext...lt#VML')
22:07:51 File '/home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/extensions/Kartographer/lib/mapbox/#default#VML' referenced by 'mapbox' must exist.
22:07:51 Failed asserting that file "/home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/extensions/Kartographer/lib/mapbox/#default#VML" exists.
22:07:51 
22:07:51 /home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/tests/phpunit/structure/ResourcesTest.php:23
22:07:51 /home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/tests/phpunit/MediaWikiTestCase.php:400
22:07:51 /home/jenkins/workspace/mwext-testextension-hhvm-jessie/src/maintenance/doMaintenance.php:111

I think it fails because of this line shape.style.behavior = 'url(#default#VML)'; in mapbox-lib.js ....

Help appreciated 😝

Fix the core or patch Mapbox?

Fix the core or patch Mapbox?

That's no mapbox bug though.
I guess I'll have to fix the core... 😓

I mean even if there's no bug with MB, we could still alter it to avoid triggering the failure. LMK if my help needed with core:)

Tested after rebase, and it seems to me that all features work . We only need to make CI tests pass.

debt removed JGirault as the assignee of this task.Jun 6 2017, 7:53 PM
debt moved this task from Stalled/Waiting to Backlog on the Maps-Sprint board.
debt removed a project: Patch-For-Review.
debt added subscribers: JGirault, debt.

Moving to backlog until such time that we can take this up again.

Looks like there are only tests to fix - moving off the sprint board as the Discovery team won't be able to finish this work at this time.

TheDJ added a subscriber: TheDJ.Jul 23 2017, 12:37 AM

Leaflet 1.1.0 is out now btw.

TheDJ added a comment.EditedAug 9 2017, 8:35 PM

retested this and still seems to work. Testcases are now OK, I think we can merge this.

Checked with @debt before merging.

Change 317546 merged by jenkins-bot:
[mediawiki/extensions/Kartographer@master] Upgrade mapbox.js to v3.0.1 (upgrade leaflet to 1.0.2)

https://gerrit.wikimedia.org/r/317546

Yurik added a comment.Aug 9 2017, 9:22 PM

I recently implemented a leaflet+vega integration, which allows any vega graph to be drawn on top of a map. Once Vega 3 is reviewd (T172938), it can be added to Kartographer. This would solve many wikivoyage requests too - like having unlimited number of data points on a map.

debt moved this task from Backlog to Needs review on the Maps-Sprint board.
debt added subscribers: Gehel, Pnorman.

@Gehel and @Pnorman - can you take a quick look and see if this merged patch can be released into production?

TheDJ closed this task as Resolved.Aug 21 2017, 10:09 AM
TheDJ claimed this task.
debt moved this task from Needs review to Done on the Maps-Sprint board.Aug 24 2017, 9:48 PM

Thanks, @TheDJ !