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.

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: support create a desktop for link #89

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

jeesk
Copy link

@jeesk jeesk commented Mar 25, 2024

It should be allowed to create a website or deeplink shortcut for Windows, translated into English

src/validation.js Outdated Show resolved Hide resolved
@TheJaredWilcurt
Copy link
Member

What do you mean by "deeplink"?

Co-authored-by: The Jared Wilcurt <TheJaredWilcurt@users.noreply.github.com>
@jeesk
Copy link
Author

jeesk commented Mar 25, 2024

What do you mean by "deeplink"?

Custom protocols, such as Lazycat, can be used for Deeplink links lazycat://index/1?key1=value1&key2=value2. In general, Deeplink is a protocol registered by the application itself, and the application can be opened through Deeplink.

Our Electron application has registered some of our own Deeplinks, please refer to the following address: https://www.electronjs.org/docs/latest/tutorial/launch-app-from-url-in-another-app

Comment on lines 398 to 399
windowsFilePath = helpers.resolveWindowsEnvironmentVariables(windowsFilePath);
windowsFilePath = this.resolvePATH(windowsFilePath);
Copy link
Member

Choose a reason for hiding this comment

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

  • There needs to be some basic safety checking for this feature (see suggestion below).
  • Also the isLink should be checked as a optional boolean defaulting to false in the validation file.
  • There needs to be unit tests created to ensure the repo stays at 100% test coverage.
  • The documentation in the README needs updated as well to convey this new feature.
Suggested change
windowsFilePath = helpers.resolveWindowsEnvironmentVariables(windowsFilePath);
windowsFilePath = this.resolvePATH(windowsFilePath);
if (options.windows.isLink) {
if (
typeof(windowsFilePath) !== 'string' ||
!windowsFilePath.includes('://')
) {
helpers.throwError(options, 'WINDOWS filePath must be a string of a URL when isLink is true:' + windowsFilePath);
delete options.windows;
}
return options;
}
windowsFilePath = helpers.resolveWindowsEnvironmentVariables(windowsFilePath);
windowsFilePath = this.resolvePATH(windowsFilePath);

@TheJaredWilcurt
Copy link
Member

Custom protocols on desktop apps aren't a new concept, just didn't know that Electron is giving them the name "deep links", which I've only heard in reference to Mobile apps, but the concept is the same. I wish there was a generic Node module to register custom protocols on each platform.

Anyways. I added some comments as to the rest of the work needed before this PR can be merged. If you prefer not to do it, you can create an issue instead and I'll get to it at some point.

@jeesk
Copy link
Author

jeesk commented Mar 26, 2024

Custom protocols on desktop apps aren't a new concept, just didn't know that Electron is giving them the name "deep links", which I've only heard in reference to Mobile apps, but the concept is the same. I wish there was a generic Node module to register custom protocols on each platform.

Anyways. I added some comments as to the rest of the work needed before this PR can be merged. If you prefer not to do it, you can create an issue instead and I'll get to it at some point.

good job, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants