-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
feat: marker groups #5113
feat: marker groups #5113
Conversation
bec3380
to
e2a8e4a
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #5113 +/- ##
==========================================
+ Coverage 86.71% 86.79% +0.07%
==========================================
Files 556 558 +2
Lines 43492 43604 +112
Branches 6762 6775 +13
==========================================
+ Hits 37714 37846 +132
+ Misses 5778 5758 -20
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
||
// this caps total amount of markers at 500 - should it maybe be done only for rendered markers? | ||
// on top of it, do we need to cap the length of a rendered marker range to avoid performance issues? | ||
MarkerGroup.prototype.MAX_MARKERS = 10000; |
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.
In the comments it says it's capped at max 500 markers but here it's set to 10k. Do we know what the performance impact is when rendering 10k markers?
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.
@nightwing ^^
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.
Now we do not render 10k markers, only markers on screen are rendered which should not cause performance issues,
The description of the API in the PR description doesn't match the current implementation as far as I can tell. |
updated the description a bit |
e2a8e4a
to
2580b19
Compare
2580b19
to
1780348
Compare
1780348
to
6a9d56f
Compare
Before merging, can we change the PR title to something closer to what this PR does? If users see |
Issue #, if available: #2720
Description of changes:
MarkerGroup
for adding multiple markers for an edit session. This could be useful for inline diagnostics or displaying occurrence.How could it be used:
MarkerGroup
is created by passing edit session object to it.HoverTooltip
instance could manage displaying multipleMarkerGroup
s. (see demo source)By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.