-
Notifications
You must be signed in to change notification settings - Fork 28
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
Omnibox keyword search fix, DIY sidePanel, and PowerShell build script #102
base: dev
Are you sure you want to change the base?
Conversation
Signed-off-by: VaiMalaviya <[email protected]>
Signed-off-by: VaiMalaviya <[email protected]>
Signed-off-by: VaiMalaviya <[email protected]>
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.
sorry, I can't run sh script on windows that's why i added
], | ||
"background": { | ||
"service_worker": "./background.js", | ||
"type": "module" | ||
}, | ||
"omnibox": { | ||
"keyword": "lk" | ||
"keyword": "lw" |
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 be "lw" considering it is linkwarden and not linkding's "lk"
@@ -12,6 +12,7 @@ import OnClickData = chrome.contextMenus.OnClickData; | |||
import OnInputEnteredDisposition = chrome.omnibox.OnInputEnteredDisposition; | |||
|
|||
const browser = getBrowser(); | |||
let configuration = null; // this js needs init function which initializes the configuration variable thus solving many lines in this file which repeadiatly uses getConfig() it might be better |
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 come from the java/kotlin/python so I don't know more of vite/javascript/typescript but this variable needs to be initialized in init function which should be added to this file which also reduces the getConfig() function code.
const isUrl = /^http(s)?:\/\//.test(content); | ||
const url = isUrl | ||
? content | ||
: `lk`; | ||
: `${configuration.baseUrl}/search?q=${encodeURIComponent(content)}`; // This part was taken https://github.com/sissbruecker/linkding-extension/blob/master/src/background.js |
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.
code from linkding
} catch (error) { | ||
console.error("Failed to reload the side panel:", error); | ||
} | ||
}); |
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.
ummmm.... i inspected through the background.js console it does execute (see attached image) but chrome.sidePanel.setOptions()
function doesn't reload the tab if it's already set to that for that we need to separate the sidepanel page and still i don't know vite so but i'll try to learn vite and implement it via chrome.runtime.onMessage.addListener()
which should be able to execute the reload code window.location.reload();
📝:- also you might want to block comment out the both listener functions since they do nothing it was for debug.
Additions
Modifications
Fixes
TODO