-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
Menu item to copy URL addition #80
base: master
Are you sure you want to change the base?
Conversation
childview.webContents.loadURL(windowSettings.url); | ||
}); | ||
|
||
electronLocalshortcut.register(childwin, ["F8"], () => { |
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.
Should there only be one of these blocks?
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.
Also, can we add it to the menu? It looks like we could add it to menu.js
, but I'm not sure how to access the window from there. If we put it in the menu, then Mac users will be able to override the keyboard shortcut.
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.
The way that the app is laid out, where a browser view (containing the website) is encapsulated inside of a larger window (containing the titlebar) requires the shortcut to be registered to both the view and the window. This ensures that the keyboard shortcut is capture regardless of whether the user is focused on the titlebar or the view.
However, I agree that adding listeners twice adds unnecessary bloat. It would be optimal if there is a way to move this to menu.js
to make it a global shortcut or refactor it.
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.
Ah, good points and thanks for feedback! I thought that adding it in childwindow.js
was more logical since the file would open in a child window. Thus, you would only like the link of the child window. I'll take a look next weekend as I have a final coming up Thursday!
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.
Do you think there is a way to use define these shortcuts in menu.js
instead of using electronLocalShortcut?
childview.webContents.loadURL(windowSettings.url); | ||
}); | ||
|
||
electronLocalshortcut.register(childwin, ["F8"], () => { |
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.
The way that the app is laid out, where a browser view (containing the website) is encapsulated inside of a larger window (containing the titlebar) requires the shortcut to be registered to both the view and the window. This ensures that the keyboard shortcut is capture regardless of whether the user is focused on the titlebar or the view.
However, I agree that adding listeners twice adds unnecessary bloat. It would be optimal if there is a way to move this to menu.js
to make it a global shortcut or refactor it.
It looks like you can add shortcuts to the menu items, like |
Problem
Removed .idea and .vscode folder. Those are user specific and should not be share through git since it contains their own personalized settings.
Resolves #79 .
Solution
How did you solve the problem?
added 5 lines of code to copy the url onto the clipboard.
Before & After Screenshots
BEFORE:
[insert screenshot here]
AFTER:
[insert screenshot here]
Other changes (e.g. bug fixes, UI tweaks, small refactors)
Deploy Notes
Notes regarding deployment of the contained body of work. These should note any
new dependencies, new scripts, etc.
New environment variables:
env var
: env var detailsNew scripts:
script
: script detailsNew dependencies:
dependency
: dependency detailsNew dev dependencies:
dependency
: dependency details