New "bookmark" plugin allows hotkey fast-travel to specific steps#852
New "bookmark" plugin allows hotkey fast-travel to specific steps#852henrikingo merged 4 commits intoimpress:masterfrom
Conversation
similar to "click", we can now fast travel using hotkeys e.g. 1 2 3
|
i will make the linter happy |
|
You still have double quotes in the code that's why it complains. Please run before pushing |
janishutz
left a comment
janishutz
left a comment
There was a problem hiding this comment.
Thank you for creating this plugin. My suggestion is to add some more in-code documentation, so it is easier for a new user to read the code and understand what happens
| // window.console.log(`bookmark: recognizing hotkey ${event.code} goes to ${dest}`) | ||
| var newTarget = document.getElementById( dest[0] ); // jshint ignore:line | ||
|
|
||
| // Window.console.log(`bookmark: recognizing hotkey ${event.code} goes to ${dest}`) |
There was a problem hiding this comment.
Do you really want to leave the debug code commented out in here?
There was a problem hiding this comment.
i have taken it out, will push a new rev. Still working on the test failure.
| div.dataset.bookmarkKeyList.split( " " ).forEach( ( k ) => { | ||
| if ( hotkeys.hasOwnProperty( k ) ) { | ||
| hotkeys[ k ].push( div.id ); | ||
| } else { hotkeys[ k ] = [ div.id ]; } } ); } } ); |
There was a problem hiding this comment.
Can you move those closing brackets to separate lines? This makes the code more legible for beginners, imo. Also some explanation might be needed for the code here, if someone with less coding (well js, anyway) knowledge than you have reads this code.
There was a problem hiding this comment.
lol i'm surprised the linter even let me do that … haha
I don't really think there's anything to be gained from adding a bunch of punctuation over multiple lines… my preference is to keep things as short as possible so you can see everything on one screen. They used to say "you can write Fortran in any language", now I suppose they should say "you can write Python in any language" 😺
There was a problem hiding this comment.
The linter is not properly configured for that. I have an issue open to talk about moving to eslint and how to configure it
| @@ -18,48 +22,51 @@ | |||
| * Copyright 2016-2017 Henrik Ingo (@henrikingo) | |||
There was a problem hiding this comment.
You can credit yourself here as well!
|
|
||
| If you assign the same key to multiple steps, that hotkey will cycle among them. | ||
|
|
||
| WARNING: It's up to you to avoid reserved hotkeys H, B, P, ?, etc. |
There was a problem hiding this comment.
Couldn't you just disallow the usage of those keys?
There was a problem hiding this comment.
Don't think so, it would clobber the other plugins … I'll think about how to resolve the interaction effects.
There was a problem hiding this comment.
I think this is ok. If you want to override a reserved key, just do it.
|
You know, the weird thing that's going on, is that both my PRs, which are generally unrelated, both are failing tests in similar ways … maybe a curse is following me around |
|
if I could get this to run after init, the |
|
so, I did run |
|
ah, null !== undefined JS is the best language EVAR |
|
Fun fact, this is actually one of my most favourite (and at the same time most hated) features of js |
|
Thanks for helping me get this PR shipshape, I appreciate it. I am building an app involving this codebase and will keep an eye out for problems. If there are no problems I will follow up in a while and maybe we can have another think about integrating this into master. |
|
Okay, great! Can you post a link to your project here, if it's open source? |
I will … once it is more presentable … still trying to figure out a bunch of things. |
3 participants
resolves #850