-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added a keyboard shortcut for adding files #2395
Added a keyboard shortcut for adding files #2395
Conversation
Hi @lindapaiste, could you review my pull request and provide feedback? Thanks! |
cc9cb47
to
7d4d7f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts:
Shift-N
alone will not work because it means that user cannot type a capital letter "N" anywhere in their code.- We do override some built-in browser commands already. For example If you type
Ctrl-H
while focused on the editor it will open the find/replace modal. If you aren't focused on the editor then it does the browser default which is to open the browser history. So we potentially could overrideCtrl-N
? Unless there is something special about that command. - You are missing the implementation of the handler. It would go here if it applies to the entire window or here if it applies only when the cursor is in the editor.
|
7d4d7f4
to
c08181d
Compare
Thanks for researching this!
Your implementation looks good 👍 (with the exception of a I agree that we need to come to a consensus on what the keys should be. My personal vote is for Ctrl-Shift-N. @raclim -- any thoughts? |
Of course, I understand your point. However, it's worth noting that |
@lindapaiste @sdivyanshu90 Thanks for your work/thoughts on this so far! For adding new files, I'm personally partial to using a command that involves the letter 'N' because there's already a pre-existing precedent with it on other softwares/platforms. I also could be wrong about this, but I think I'm wondering if |
Hey @raclim , You're right about If this works for you, let's proceed with this option. |
Hi @raclim and @lindapaiste, |
Hello @raclim and @lindapaiste, |
Hi @catarak , @raclim and @lindapaiste , |
Hey @raclim and @lindapaiste, can you please review the pull request? |
5534d69
to
74ae0ca
Compare
Hey @raclim and @lindapaiste, can you please review the pull request? |
Hi @lindapaiste , |
Dear @lindapaiste and @raclim , I hope you're well. I submitted a pull request #2395 on September 2nd for p5.js. I appreciate your time and expertise in maintaining the project. I've addressed the feedback and am eager for your review. The process has been paused since September 9th. Could you kindly spare some time to review my PR? Your feedback is crucial for progress. Thank you for your attention, and I look forward to your insights. |
Hey @raclim and @lindapaiste, can you please review the pull request? |
Sorry for the delay in getting back to this and thanks so much for your patience! We have a small team and had quite a lot of PRs recently! Looking over it at a glance, it looks good to me so far. Since we're currently on break for the holidays, I'll try to have a more in-depth review around early January. Thanks again for your patience! |
Hello, Thank you so much for your prompt response! I completely understand about the delay, and I appreciate your transparency about your team's workload. I'm glad to hear that the initial review looks good to you. No worries about the more in-depth review – take your time, especially during the holidays. I'm grateful for your efforts, and I'm looking forward to hearing more in early January. Thanks again and happy holidays! |
d3990e2
to
257ff59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for your work and patience on this! I'm sorry that it took so long to finally get back to this!
Although Alt-N is a bit shorter than Ctrl-Alt-N, I ended up using the latter after doing some research, as it seemed like Ctrl-Alt-N is used more commonly to open "new" files or projects on other platforms/software over Alt-N.
Thank you so much and I'm really sorry again that it took so long!
Thank you for getting back to me! No problem about the delay—I understand these things take time. I appreciate your consideration and research on the shortcut. Using Ctrl-Alt-N makes sense if it aligns better with common conventions for opening new files or projects. Thanks again, and no worries about the timing! |
} else if (isCtrl && e.altKey && e.code === 'KeyN') { | ||
// specifically for creating a new file | ||
handlers.current[`ctrl-alt-n`]?.(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would stay away from e.code
and prefer e.key
. It only makes a difference if the user has a non-standard keyboard layout. e.code
is basically "the key in the 'N' position" whereas e.key
is "the key that types 'N'".
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key
I guess I only built this hook to support ctrl-
and ctrl-shift-
and didn't do anything with alt
. But we want it to support any ctrl-alt-LETTER
and not just ctrl-alt-n
. This should do it:
} else if (isCtrl && e.altKey) {
handlers.current[`ctrl-alt-${e.key.toLowerCase()}`]?.(e);
Fixes #1292
Changes:
Shift + N
.I have verified that this pull request:
npm run lint
)npm run test
)develop
branch.Fixes #1292