feature(gutter): add experimental ability to replace fold icon with custom for a specific row #5787
Conversation
nlujjawal
force-pushed
the
gutter_icon
branch
2 times, most recently
from
dccd10f to
0dc96b2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5787 +/- ##
==========================================
+ Coverage 87.24% 87.32% +0.07%
==========================================
Files 604 605 +1
Lines 44305 44527 +222
Branches 7265 7287 +22
==========================================
+ Hits 38656 38881 +225
+ Misses 5649 5646 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We can do this in a follow-up PR, but we should also make sure these custom widgets can get triggered by users using the keyboard gutter handler. |
src/layer/gutter.js
Outdated
| hideFoldWidget(row) { | ||
| const cells = this.$lines.cells; | ||
| const cell = cells[row]; | ||
| if (cell && cell.element) { | ||
| const foldWidget = cell.element.childNodes[1]; | ||
| if (foldWidget) { | ||
| dom.setStyle(foldWidget.style, "display", "none"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Shows the fold widget/icon from a specific row in the gutter | ||
| * @param {number} row The row number from which to show the fold icon | ||
| */ | ||
| showFoldWidget(row) { | ||
| const cells = this.$lines.cells; | ||
| const cell = cells[row]; | ||
| if (cell && cell.element) { | ||
| const foldWidget = cell.element.childNodes[1]; | ||
| if (foldWidget) { | ||
| dom.setStyle(foldWidget.style, "display", "inline-block"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
should these 2 be a toggleFoldWidget(row, show)?
There was a problem hiding this comment.
it will lead a lot of if else and can lead to less readability
There was a problem hiding this comment.
only lines 607 and 622 are uncommon and it would be a simple ternary, do you still think it would be less readable?
There was a problem hiding this comment.
if we merge them, I would maybe call it something like toggleFoldWidgetVisibility to make sure it's clear we are toggling the visibility of the widget and are not folding/unfolding the code at that widget.
There was a problem hiding this comment.
There is 2 lines(one is if condition) of difference now in both the functions and if we add toggle logic,
There will be 2 extra nested if's with two more of the if's which will always exists which I think won't be that much readable.
| $getGutterCell(row){ | ||
| // contains only visible rows | ||
| const cells = this.$lines.cells; | ||
| const visibileRow= this.session.documentToScreenRow(row,0); |
There was a problem hiding this comment.
documentToScreenRow on large documents can be much slower than searching row in the cells array
There was a problem hiding this comment.
Searching the cells doesn't work properly in case when the line is folded and you move the cursor to the right of the fold symbol in the code. It doesn't work because at that time cursor is on a hidden row.
I tried creating a large document of 50K lines and 100 custom widgets at a time, current code seems to be working fine without any lag.
nlujjawal
changed the title
Do you see any concern with the code or its the feature you dont like? |
Issue #, if available:
Description of changes:
Adding Experimental capability in gutter to replace the fold icon with the custom icon.
Custom widget Icon can be added which will hides the fold Icon and show custom widget icon.
And custom widget Icon can be removed which will remove the custom widget and displays the fold icon.