-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat: adds a hotkey to open dev menu #167
feat: adds a hotkey to open dev menu #167
Conversation
@watadarkstar is attempting to deploy a commit to the software-mansion Team on Vercel. To accomplish this, @watadarkstar needs to request access to the Team. Afterwards, an owner of the Team is required to accept their membership request. If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account. |
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.
Hello, I left some hopefully helpful inline comments.
regarding the correct binding, I don't have an opinion witch biding is better, but I think it's good idea to disable cmd
no matter what we choose. You wiil find that keylisteners are defined in file named Preview.ts
@filip131311 Thanks for the review. I will work on these comments when I get a chance. Maybe this weekend or late next week. |
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.
In line
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'd be ok with merging this as is w/o focusing on other aspects like displaying hotkey binding in the UI etc. However, I since I don't have experience with commands that have keys it'd be important for me to understand:
- if key is specified in package.json does it serve as default key for that command
- What if there is conflict with some other command (I personally use cmd+d to delete lines)?
- Can we leave it empty and just allow users to configure it?
- Is the key combination going to work across all editors or only when focused on the IDE panel?
I agree, I also use CMD + D for deleting lines. I will change it back to Lets slim this down, we can add the hotkey binding in the UI in another PR. To answer your questions:
Yes.
We should try to avoid conflicts, hence why I choose
Yes you can leave it empty. My recommendation is to have it default to
It will work across all editors not just the IDE panel. |
@kmagiera @filip131311 I'm working on this PR today. Going to try to get it in a ready place but I say let's keep it simple and focus on the "hotkey functionality". @filip131311 You can complete the cog wheel UI work in another PR. |
@watadarkstar Hey, It's great news are you open to work on a cog wheel functionality in a seperate PR? |
Yeah potentially I can do it but it will take some time as my schedule is busy. I only work on open source every other Friday. Do you have Discord @filip131311 ? I have a few questions on this static method refactor I want to ask you. My discord is: |
@filip131311 I'd like to avoid disabling the keybinding because |
@filip131311 @kmagiera This is ready to be reviewed again. I have cleaned up the code, simplified it, removed the static methods and tested it fully. Here is a Loom video demoing the feature. |
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.
Thanks, this looks good now.
An owner of the software-mansion Team on Vercel declined @watadarkstar's request to join. In order for their commit to be deployed, @watadarkstar must push and request access again. |
Description
Closes #148
Loom video demo
A few notes and questions:
cmd + ctrl + z
because I didn't want to interfere withcmd + d
which is already a command that is handled by the simulator (also becausecmd + d
is a bit buggy on Expo projects and opens a different menu than this one). Another reason I choose this is becausecmd + ctrl + z
is the default to shake the device on the simulator for iOS. We can change this if needed as I'm sure I may get some push back to usecmd + d
orcmd + ctrl + d
.openDevMenu
keybindingTODOs
cmd + ctrl + z
as the default keybinding see my rationale here)How to test
cmd + ctrl + z
at any pointopenDevMenu
keybindingScreenshot