Refactor jump-to functionality, remove unused code #423
Conversation
|
Heavy! Let me know if I can assist in any way. |
|
That would be it. I will try to merge PRs tomorrow and do a release before Monday. Feel free to review (though I will likely defer any major changes by opening issues out of comments). I am not sure how to test symlinking (only Ubuntu? all OSes?) - though not aiming to do this now (not enough time). |
👀
Should work fine on unixen... wouldn't bother on windows... guess i'll see more when i get there. If this is about the |
atest/05_Features/Jump.robot
Outdated
| Click Element ${sel} | ||
| ${original} = Measure Cursor Position | ||
| Capture Page Screenshot 01-ready-to-jump.png | ||
| Click Element ${sel} modifier=CTRL |
There was a problem hiding this comment.
You think 🍎 people will expect ⌘ ?
There was a problem hiding this comment.
Indeed, it seems so there was a failure on this. Should we mention it in readme, or is it typically obvious to 🍎 people?
There was a problem hiding this comment.
Interesting post: https://nuclide.io/blog/2017/02/27/Command-Click-You-Have-One-Job/
I forgot about the multi-cursor placement! Ah, this is difficult to settle. If we keep Ctrl as default it might break workflow for some users; at least there is a setting for this; though indeed many IDEs use Ctrl + click for jump, and I think that fewer use for placement of additional cursors - so that seems like a tie to me.
| @@ -38,13 +38,14 @@ | |||
| "private": true, | |||
| "scripts": { | |||
| "bootstrap": "jlpm --no-optional --prefer-offline && lerna bootstrap && jlpm lint && jlpm clean && jlpm build", | |||
| "build": "jlpm build:schema && jlpm build:completion-theme && jlpm build:theme-material && jlpm build:theme-vscode && jlpm build:meta && jlpm build:ws", | |||
| "build": "jlpm build:schema && jlpm build:completion-theme && jlpm build:theme-material && jlpm build:theme-vscode && jlpm build:jump && jlpm build:meta && jlpm build:ws", | |||
There was a problem hiding this comment.
honestly would love to get rid of this command, as build:meta should do it all...
| } | ||
| } | ||
|
|
||
| jumpers.set('fileeditor', FileEditorJumper); |
There was a problem hiding this comment.
legacy, keeping as is for now, but needs a refactor in future.
| @@ -29,10 +29,11 @@ | |||
| }, | |||
| "scripts": { | |||
| "build": "jlpm build:schema && tsc -b", | |||
| "build:schema": "jlpm build:schema-backend && jlpm build:schema-completion && jlpm build:schema-hover && jlpm build:schema-diagnostics && jlpm build:schema-syntax_highlighting", | |||
| "build:schema": "jlpm build:schema-backend && jlpm build:schema-completion && jlpm build:schema-hover && jlpm build:schema-diagnostics && jlpm build:schema-syntax_highlighting && jlpm build:schema-jump_to", | |||
There was a problem hiding this comment.
this is getting long... could be lerna run build:schema and push the various schema builds into their respective packages?
There was a problem hiding this comment.
yes, good idea; maybe not on this PR because the features are not packages just yet.
|
Other than the OSX thing, LGTM aside from minor, unimpactful word suggestions. There's also the potential to make some of the schema building defined where it's used (rather than an increasingly long JSON line) and use |
|
Changing to Alt + click as default to avoid breaking UX for users using multi-cursor feature (this is how I did it in https://github.com/krassowski/jupyterlab-go-to-definition too). |
@krassowski/jupyterlab_go_to_definition; these functions were never used injupyterlab-lspand can be still accessed from original@krassowski/jupyterlab_go_to_definition:@krassowski/jupyterlab_go_to_definitionto@krassowski/code-jumpersmove jumpers up the source tree to the/srcmodifierKeyand bindingstest opening external files (symlinked)not this timeReferences
Fixes #370, fixes #372.
Code changes
TBD
User-facing changes
TBD
Backwards-incompatible changes
@krassowski/code-jumpers@krassowski/code-jumpersChores