Updated keymap and handlerName to new key method

[SteliosPhanartzis]

References #211

Left comments next to associated key mappings.

[welcome]

Thanks for opening this pull request! Dangerbot will test out your code and reply in a bit with some pointers and requests.
There may be some errors, but don't worry! We're here to help! 👍🎉😄

[sashadev-sky]

Hi, thank you for the updates! This looks good and is ready for merge @jywarren

Two quick tips for the future:

• I happened to be looking through the PRs for this repo and noticed yours, but note typically you want to reference the original issue at the top of your PR description like this Incorporate newer key API method in place of the deprecated which method #211
• For each PR, you want to create a new feature branch and push your code there instead of working from your main. This will allow you to work on multiple features at once and swap between them

[SteliosPhanartzis]

All right, I'll note this for future PRs. Thank you for letting me know 😄

[rexagod]

Okay two things to note here.

• It'd be perhaps better to remove the comments since we used those to identify the numerical keycodes before we implemented the which method, so now the keycodes and comments are identical and we really want to keep the code clean whenever we have an opportunity to.
• You need to build your code into the dist/ folder by executing grunt build on your local once you are done with the final changes, I guess I should have mentioned this above, sorry!

@SteliosPhanartzis This is some really nice work, and with the above changes, we shouldn't have any problems merging this. Thank you! 👍

[sashadev-sky]

@rexagod good point! Just to add on to that, lets keep the comments for these two hotkeys (first 2):

'Backspace': '_removeOverlay', // backspace windows / delete mac
'Delete': '_removeOverlay', // delete windows / delete + fn mac

I personally need this as a reminder that windows backspace === delete on a mac

but we can delete the rest!

[rexagod]

@sashadev-sky That makes sense. @SteliosPhanartzis Please make the aforementioned changes. Thanks!

[SteliosPhanartzis]

Alright cool, I'll make the changes tonight and push them up.
I did this now. Also not sure why, package-lock.json was uploaded even though it's in the .gitignore

[rexagod]

@SteliosPhanartzis This looks good. Just one final change, can you please revert your latest commit (Delete package-lock.json)? It somehow got introduced in the .gitignore of this library (we are investigating that) and the npm guidelines clearly state that it must be committed at all times. Sorry for any inconvenience caused, you're doing a great job! Thank you!

[sashadev-sky]

and the npm guidelines clearly state that it must be committed at all times.

@rexagod not disagreeing with you just wondering can you link the guidelines please because I need to read up about this myself

@SteliosPhanartzis If you want I think you can just go to Files changed in the browser and click the trash can next to the file, and then it will let you commit from the browser. Sometimes editing in the browser directly in github is a lot simpler, just note if you do this you must pull in your remote changes locally using git pull origin <branch_name> if you plan on continuing to work on the PR afterwards locally

• also you introduced it for the first time in the commit 'Built code into dist/ folder' not the last one I think you might need to trash it from both

[rexagod]

@sashadev-sky It can be found here.

---

This file is intended to be committed into source repositories, and serves various purposes:

• Describe a single representation of a dependency tree such that teammates, deployments, and continuous integration are guaranteed to install exactly the same dependencies.

• Provide a facility for users to “time-travel” to previous states of node_modules without having to commit the directory itself.

• To facilitate greater visibility of tree changes through readable source control diffs.

• And optimize the installation process by allowing npm to skip repeated metadata resolutions for previously-installed packages.

[sashadev-sky]

@SteliosPhanartzis hey, good job with this! A big PR was recently merged so that why is you're seeing all of these conflicts now. I will try to rebase for you this time around because you opened the PR before these changes

[sashadev-sky]

Please re-add the two _removeOverlay lines.

[sashadev-sky]

please re-add the _toggleIsolate line for e as well

[sashadev-sky]

@SteliosPhanartzis Ok I rebased this so as you can see now the "changed files" only displays your changes so we can see them clearly. By rebasing, your branch now knows what commits are really new coming from you. (Probably not the most exact explanation of what happened but you can read up about this if you are curious).

So looking at your updates, it looks like you have left out some keybindings when doing the transition to the new syntax. Please readd them! I commented above so you can easily see which ones. There is already an issue open which I will address in a separate PR to fix keybinding inconsistencies (I see where you were coming from deleting one of the _toggleIsolate), but in this PR we are just transferring from one syntax to another!

[sashadev-sky]

@rexagod So I think it should be committed only if there were updates made to dependencies that need to be shown there - otherwise users should not be individually committing package-lock.json to every single one of their PR's, if the PR has nothing to do with dependency updates.

@jywarren can you please chime in here?

@SteliosPhanartzis I will pull your PR again and take care of the package-lock.json issue for you. What you have to do is just address my comment above about the misplaced keybindings.

[sashadev-sky]

@SteliosPhanartzis Ok you are good to go! Reminder to run git pull origin main before you start working to make sure these changes are present in your local repo as well

[sashadev-sky]

@SteliosPhanartzis Due to lack of response (~ 13 days) I am closing this PR. It is essentially a part of what I am addressing in my current PR #229, so I am going to resolve it there. Otherwise, you will find more code conflicts here and this branch will go stale.

So sorry to close this one on you! We totally understand it happens and I am always willing to make you a new FTO in this repo if you just mention me here.

[jywarren]

Some guidance on package-lock.json is here! hexojs/hexo#3370 -- so, we can better guarantee compatibility of specific dependency versions (not just semver.org ranges), AND dependabot uses package-lock.json to bump versions for security issues pretty regularly.

[jywarren]

Also it's recommended here: https://docs.npmjs.com/files/package-locks#using-locked-packages

[jywarren]

Thank you!!!

[jywarren]

Oh, i'm sorry - your question is not about whether it should be /tracked/, but whether it should be /gitignored/ -- yes, I'm OK with gitignoring it!

[sashadev-sky]

@rexagod does that make sense to you?

[rexagod]

Just a bit confused on how are going to track lock files while ignoring them at the same time?

[jywarren]

We can rely on Dependabot for most of the updating. But if we do a big breaking change, we will have to `git add -f` and commit them manually!

…

On Tue, Apr 30, 2019 at 5:57 PM Pranshu Srivastava ***@***.***> wrote: Just a bit confused on how would we track lock files while ignoring them at the same time? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#216 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J47ETT4MKYKRMAEV4TPTC6ATANCNFSM4HGEL4LQ> .

[rexagod]

Okay, this makes sense. Thank you for clearing this out, @jywarren!