-
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
Custom scrollbar with gutter indicators #4905
Conversation
…display gutter indicators
Codecov ReportBase: 85.76% // Head: 85.85% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #4905 +/- ##
==========================================
+ Coverage 85.76% 85.85% +0.08%
==========================================
Files 539 542 +3
Lines 40723 41131 +408
Branches 6442 6514 +72
==========================================
+ Hits 34928 35313 +385
- Misses 5795 5818 +23
Flags with carried forward coverage won't be shown. Click here to find out more.
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 at Codecov. |
src/layer/decorators.js
Outdated
y2 = Math.round(yCenter + this.halfMinDecorationHeight); | ||
} | ||
|
||
switch (annotations[i].type) { |
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.
can we instead simplify this to
ctx.fillStyle = colors[annotations[i].type] || null
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.
Yes, that's definitely better, corrected!
var annotations = this.renderer.session.$annotations; | ||
ctx.clearRect(0, 0, this.canvas.width, this.canvas.height); | ||
if (annotations) { | ||
annotations.forEach(function (item) { |
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.
can we do something like
can we instead simplify this to
item.typeNumber = typeNumbers[item.type] || null
src/layer/decorators.js
Outdated
var ctx = this.canvas.getContext("2d"); | ||
|
||
function compare(a, b) { | ||
if (a.typeNumber < b.typeNumber) return -1; |
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.
I'd better rename it typeNumber
-> priority
src/layer/decorators.js
Outdated
|
||
}; | ||
|
||
this.compensateFoldRows = function (currentRow, foldData) { |
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.
It seems like this logic is not covered with tests, can we add tests for this?
src/layer/decorators.js
Outdated
|
||
for (let i = 0; i < annotations.length; i++) { | ||
let currentRow = annotations[i].row; | ||
let compensateFold = this.compensateFoldRows(currentRow, foldData); |
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.
I'd rename currentRow to just row to avoid confusion with current cursor position
src/layer/decorators.js
Outdated
|
||
}; | ||
|
||
this.compensateFoldRows = function (currentRow, foldData) { |
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.
I'd rename currentRow to just row to avoid confusion with current cursor position
eb78690
to
f7b1b5c
Compare
Issue #, if available:
Description of changes:
Adds custom scrollbar with indicators for annotations
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.