-
Notifications
You must be signed in to change notification settings - Fork 414
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: add keyboard shortcut to quit the app on Windows #1202
Conversation
@ykris45, Please format your code with Prettier: https://github.com/lbryio/lbry-app/blob/master/CONTRIBUTING.md#code-formatting Also, I would recommend you to use a text editor and not a word processor like WordPad to develop. A simple to use and a good one is: https://code.visualstudio.com/ |
@IGassmann that is correct bro |
or i use VS Code Prettier Formatter guys |
i fixed link to this pr on changelog |
@tzarebczan done sir |
CHANGELOG.md
Outdated
@@ -44,7 +44,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/). | |||
* LBRY URLs not working on Linux ([#1120](https://github.com/lbryio/lbry-app/issues/1120)) | |||
* Fix Windows notifications not showing ([#1145](https://github.com/lbryio/lbry-app/pull/1145)) | |||
* Fix export issues ([#1163](https://github.com/lbryio/lbry-app/pull/1163)) | |||
|
|||
* Fix Keyboard Shortcut to Quit APP ([#1163](https://github.com/lbryio/lbry-app/pull/1202)) |
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.
Please rename this as "Add keyboard shortcut to quit the app on Windows" under an unreleased section and not under "Fix".
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.
@IGassmann i cannot see where a unreleased section
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.
You have to add one.
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.
@IGassmann like this guys
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.
572d68d done
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.
It also needs to be under an Added subsection. Also, notice that the pull request number doesn't match the link number.
src/main/menu/setupBarMenu.js
Outdated
label: 'Close', | ||
accelerator: 'CmdOrCtrl+Q', | ||
role: 'quit', | ||
}, |
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.
This shouldn't be under the help menu. Add it to a new submenu "File".
src/main/menu/setupBarMenu.js
Outdated
@@ -42,6 +42,11 @@ export default () => { | |||
} | |||
}, | |||
}, | |||
{ | |||
label: 'Close', |
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.
Remove the label "Close" as it should be automatically specified by the role.
based on igassman requested changes comment
i think this mission completed |
src/main/menu/setupBarMenu.js
Outdated
submenu: [ | ||
{ role: 'minimize' }, | ||
{ role: 'close' }, | ||
{ role: 'quit', accelerator: 'CmdOrCtrl+Q' }, |
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.
This has to be under a submenu called ”File" and not the window one.
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.
@IGassmann like this guys
{
role: 'file',
submenu: [{ role: 'quit', accelerator: 'CmdOrCtrl+Q' }],
},
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.
or do you mean submenu label : 'file'
|
CHANGELOG.md
Outdated
@@ -44,7 +49,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/). | |||
* LBRY URLs not working on Linux ([#1120](https://github.com/lbryio/lbry-app/issues/1120)) | |||
* Fix Windows notifications not showing ([#1145](https://github.com/lbryio/lbry-app/pull/1145)) | |||
* Fix export issues ([#1163](https://github.com/lbryio/lbry-app/pull/1163)) | |||
|
|||
* Fix Keyboard Shortcut to Quit APP ([#1163](https://github.com/lbryio/lbry-app/pull/1202)) |
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.
Please remove this line.
based on @IGassmann comment to my PR earlier ,