Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add keyboard shortcut to quit the app on Windows #1202

Merged
merged 11 commits into from
Mar 30, 2018
Merged

feat: add keyboard shortcut to quit the app on Windows #1202

merged 11 commits into from
Mar 30, 2018

Conversation

ykris45
Copy link
Contributor

@ykris45 ykris45 commented Mar 28, 2018

based on @IGassmann comment to my PR earlier ,

@lbryio lbryio deleted a comment Mar 28, 2018
@lbryio lbryio deleted a comment Mar 28, 2018
@lbryio lbryio deleted a comment Mar 28, 2018
@lbryio lbryio deleted a comment Mar 28, 2018
@lbryio lbryio deleted a comment Mar 28, 2018
@lbryio lbryio deleted a comment Mar 28, 2018
@ykris45
Copy link
Contributor Author

ykris45 commented Mar 28, 2018

image
image
when i press ctrl+q app closed

@IGassmann
Copy link
Contributor

@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/

@ykris45
Copy link
Contributor Author

ykris45 commented Mar 28, 2018

@IGassmann that is correct bro
image

@ykris45
Copy link
Contributor Author

ykris45 commented Mar 28, 2018

or i use VS Code Prettier Formatter guys

@lbryio lbryio deleted a comment Mar 28, 2018
@lbryio lbryio deleted a comment Mar 28, 2018
@lbryio lbryio deleted a comment Mar 28, 2018
@lbryio lbryio deleted a comment Mar 28, 2018
@lbryio lbryio deleted a comment Mar 28, 2018
@lbryio lbryio deleted a comment Mar 28, 2018
@ykris45
Copy link
Contributor Author

ykris45 commented Mar 28, 2018

i fixed link to this pr on changelog

@ykris45
Copy link
Contributor Author

ykris45 commented Mar 29, 2018

@tzarebczan done sir

@lbry-bot lbry-bot assigned IGassmann and unassigned liamcardenas and IGassmann Mar 30, 2018
IGassmann
IGassmann suggested changes Mar 30, 2018
View reviewed changes
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))
Copy link
Contributor

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".

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
@IGassmann like this guys

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

572d68d done

Copy link
Contributor

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',
},
Copy link
Contributor

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',
Copy link
Contributor

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.

@lbryio lbryio deleted a comment Mar 30, 2018
@lbryio lbryio deleted a comment Mar 30, 2018
@ykris45
Update CHANGELOG.md
4b52795 
based on igassman requested changes comment
@ykris45
Copy link
Contributor Author

ykris45 commented Mar 30, 2018

i think this mission completed

IGassmann
IGassmann reviewed Mar 30, 2018
View reviewed changes
src/main/menu/setupBarMenu.js Outdated
submenu: [
{ role: 'minimize' },
{ role: 'close' },
{ role: 'quit', accelerator: 'CmdOrCtrl+Q' },
Copy link
Contributor

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.

Copy link
Contributor Author

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' }],
},

Copy link
Contributor Author

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'

@ykris45
Copy link
Contributor Author

ykris45 commented Mar 30, 2018

image
LGTM @tzarebczan ...

@ykris45 ykris45 mentioned this pull request Mar 30, 2018
Closed
@ykris45 ykris45 changed the title alternative quit APP by click CTRL+Q Add Menu / Keyboard shortcut to Quit the App on Windows Mar 30, 2018
IGassmann
IGassmann reviewed Mar 30, 2018
View reviewed changes
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this line.

@IGassmann IGassmann changed the title Add Menu / Keyboard shortcut to Quit the App on Windows feat: add keyboard shortcut to quit the app on Windows Mar 30, 2018
@IGassmann IGassmann merged commit 5ba8db6 into lbryio:master Mar 30, 2018
IGassmann pushed a commit that referenced this pull request Apr 2, 2018
docs: add changelog entry for #1202
aadd932
@ykris45 ykris45 deleted the patch-1 branch April 10, 2018 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IGassmann IGassmann IGassmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ykris45 @IGassmann @liamcardenas