Conversation
|
Thanks for the PR! I have a few pieces of work I need to get done first, but I promise to review this before the next release. Can you please ensure you've run eslint against your code and that you have updated all jsdoc comments I saw one which was missing the |
|
Sorry, I meant shift + ctrl + home, shift + ctrl + end (updated the original post). |
Update toggleSelectOffset jsdoc
|
I think the real fix is that the render method doesn't render the selection as well. If it did that it wouldn't have to worry about being waited on before making a selection. This used to be the case but since the addition of multiselect I didn't fix the webview state manager to hold more than one selection #95. I think the major fix would be some sort of decorations layer which holds whatever extra metadata related to the cell and then gets rendered on top. I don't really want to merge something that introduces more bugs so I would recommend pulling the more buggy things into a second PR and we can work on getting the first one merged. |
|
Reverted the selection with pg up, pg down, ctrl+home and ctrl+end. Let me know if more changes are needed. |
|
Once merge conflicts are resolved I'm ready to review your PR whenever you're ready for my review. Sorry I was working on a large feature for this iteration and wanted to make sure it was complete in time. I added preservation of the entire selection range via the |
Merge remote-tracking branch 'upstream/master' into improve-selection
|
@lramos15 it's ready for review, resolved the merge conflicts and updated the selection logic |
lramos15
left a comment
There was a problem hiding this comment.
Sorry for all the comments, I tried to be thorough. This is really great work and seems to work well when I tested it, I see there were a lot of random tabs or spaces I had. One thing that feels really broken to me is pressing shift with moving the arrow keys. Let me know if you have any questions.
| this.searchHandler.searchKeybindingHandler(); | ||
| } else if (event.keyCode >= 37 && event.keyCode <= 40) { | ||
| this.arrowKeyNavigate(event.keyCode, targetElement); | ||
| } else if ((event.keyCode >= 37 && event.keyCode <= 40 /*Arrows*/) |
There was a problem hiding this comment.
Ideally we would switch away from keycodes here as it's been deprecated. This is also a debt that I need to fix which will make it easier to understand what's going on here as event.keyCode === 35 doesn't convey home as well as event.key === "Home"
|
Pushed some changes addressing the feedback |
lramos15
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for the PR!
Improves the selection while dragging with the mouse, shift + click , shift + home, shift + end.
Took as reference the HxD editor
Fixes #78