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

Disable download button when dictionary is already installed #1336

Merged
merged 5 commits into from
Aug 22, 2024

Conversation

khaitruong922
Copy link

@khaitruong922 khaitruong922 commented Aug 22, 2024

Currently works for KANJIDIC and BCCWJ.
In recommended-dictionaries.json, Jitendex has its title hardcoded as Jitendex, but the title of it after installed from the recommended section is Jitendex.org [2024-08-11]. Don't know if we have control over the title of Jitendex,

image

Fixes #1333

@khaitruong922 khaitruong922 requested a review from a team as a code owner August 22, 2024 17:11
@Casheeew Casheeew added kind/bug The issue or PR is regarding a bug area/ui-ux The issue or PR is related to UI/UX/Design labels Aug 22, 2024
@jamesmaa
Copy link
Collaborator

I'm honestly fine hardcoding for the Jitendex check

@khaitruong922
Copy link
Author

Apart from dictionary title, how about checking for downloadUrl too? The url should be the same for Jitendex.

@jamesmaa
Copy link
Collaborator

Great idea! Honestly the download url might be more canonical than title

@Kuuuube
Copy link
Member

Kuuuube commented Aug 22, 2024

We probably do need to keep the check for title. Not all dicts have a download url.

@khaitruong922
Copy link
Author

My BCCWJ does not have a downloadUrl, so I think it's safe to check for both.

@MarvNC
Copy link
Member

MarvNC commented Aug 22, 2024

KANJIDIC has dynamic titles now though, are you sure?

@khaitruong922
Copy link
Author

@MarvNC You're right.
KANJIDIC and Jitendex should be disabled if installed after adding check for downloadUrl.

@jamesmaa jamesmaa added this pull request to the merge queue Aug 22, 2024
@khaitruong922 khaitruong922 changed the title Disable enabled button when dictionary is already installed Disable download button when dictionary is already installed Aug 22, 2024
Merged via the queue into yomidevs:master with commit 99710a5 Aug 22, 2024
11 checks passed
@khaitruong922 khaitruong922 deleted the fix-recommended-dict branch August 25, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui-ux The issue or PR is related to UI/UX/Design kind/bug The issue or PR is regarding a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recommended dicts still show as available to download even if they're already installed
5 participants