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

Leave url field blank when creating cards from search page #1685

Merged
merged 7 commits into from
Dec 23, 2024

Conversation

dchen5022
Copy link

Fixes #1558

Not sure how to add tests or if its necessary

@dchen5022 dchen5022 requested a review from a team as a code owner December 21, 2024 16:11
@Kuuuube
Copy link
Member

Kuuuube commented Dec 21, 2024

Does this work on edge? They have a different prefix registered but can't test this myself.

@jamesmaa
Copy link
Collaborator

Edge looks to be extension
image

And also for e-readers/android the url is kiwi-extension://

@Kuuuube
Copy link
Member

Kuuuube commented Dec 21, 2024

I'd recommend using a similar approach to this instead

return !!(url && url.includes('popup-preview.html') && !['http:', 'https:', 'ws:', 'wss:', 'ftp:', 'data:', 'file:'].includes(new URL(url).protocol));
.

@Kuuuube Kuuuube added kind/enhancement The issue or PR is a new feature or request area/anki The issue or PR is related to Anki integration labels Dec 21, 2024
@djahandarie
Copy link
Collaborator

Another option is to do something similar to the second half (after the &&) of this:

const backendPort = (!('serviceWorker' in navigator)) && window.location.protocol === new URL(import.meta.url).protocol ?

Which checks if the URL matches the protocol of import.meta.url (which will be chrome-extension or whatever is appropriate for the given browser). This seems a bit better than having a big list of protocols maybe?

@Kuuuube Do you have a preference?

@dchen5022
Copy link
Author

That looks like a cleaner solution

@Kuuuube
Copy link
Member

Kuuuube commented Dec 22, 2024

@Kuuuube Do you have a preference?

Didn't know that existed. Your idea sounds better.

@dchen5022
Copy link
Author

dchen5022 commented Dec 22, 2024

Should I add a test case for this? I'm not sure if it's possible since the expected results are in plain JSON while the window location URL is platform-dependent.

@Kuuuube
Copy link
Member

Kuuuube commented Dec 23, 2024

Taking a proper look at this, I feel like this is the wrong change to make. Why modify the url after it has been sent to handlebars?

Especially considering the replace (or slice) that needs to be done here (could also make an element and pull out the attribute but that seems bad too). And what if there's another handlebar in the future that needs to use the url? Checking for {url} isn't great either.

You could throw this in before commonData is set and it will handle this case before it even gets to the handlebars:

// Make URL field blank if URL source is Yomitan
if (URL.canParse(context.url) && new URL(import.meta.url).protocol === new URL(context.url).protocol) {
    context.url = '';
}

And if you really wanted to, stopping the empty anchor tag from being sent to anki is a simple handlebars change (it wouldn't display anything in anki regardless):

{{~#*inline "url"~}}
    {{~#if (op ">" definition.url.length 0)~}}
        <a href="{{definition.url}}">{{definition.url}}</a>
    {{~/if~}}
{{~/inline~}}

@dchen5022
Copy link
Author

Thanks for the help! I'll fix that

@Kuuuube Kuuuube added this pull request to the merge queue Dec 23, 2024
Merged via the queue into yomidevs:master with commit 120f0af Dec 23, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/anki The issue or PR is related to Anki integration kind/enhancement The issue or PR is a new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable "url" Anki field when creating cards from search page.
4 participants