-
-
Notifications
You must be signed in to change notification settings - Fork 717
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
Support for RTL text plugin 0.3.0 #4860
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #4860 +/- ##
==========================================
+ Coverage 87.66% 88.00% +0.33%
==========================================
Files 265 265
Lines 37937 37946 +9
Branches 2407 2409 +2
==========================================
+ Hits 33257 33393 +136
+ Misses 3608 3495 -113
+ Partials 1072 1058 -14 ☔ View full report in Codecov by Sentry. |
expect(workerResult.pluginURL).toBe(originalUrl); | ||
}); | ||
|
||
test('should do a full cycle of rtl loading synchronously', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how 0.2.3 works
expect(workerResult.pluginURL).toBe(originalUrl); | ||
}); | ||
|
||
test('should do a full cycle of rtl loading asynchronously', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how 0.3.0 works.
I've added both tests to make sure both option will be supported in the future.
src/source/geojson_source.test.ts
Outdated
@@ -302,6 +302,7 @@ describe('GeoJSONSource#update', () => { | |||
})); | |||
|
|||
test('forwards Supercluster options with worker request, ignore max zoom of source', () => new Promise<void>(done => { | |||
jest.spyOn(console, 'warn').mockImplementation(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unrelated to this PR, maybe we should move it to another cleanup PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted
src/ui/camera.test.ts
Outdated
@@ -3502,7 +3502,6 @@ describe('#flyTo globe projection', () => { | |||
}); | |||
|
|||
camera.on('moveend', () => { | |||
console.log(leastZoom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous message. I'd leave only the RTL text plugin changes to this PR and move all these things to a test cleanup PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a bunch of changes that seem unrelated to what's being done in this PR which I'd move to another PR but feel free to ignore
Since examples and tests all work, I'd say this is ready to go!
Good job!
Launch Checklist
Fixes #4850
CHANGELOG.md
under the## main
section.