-
Notifications
You must be signed in to change notification settings - Fork 0
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
Separate vocabulary and language menus and detect language of text #21
base: main
Are you sure you want to change the base?
Conversation
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.
Looks good in general, I left a few comments and suggestions.
var textract_base_url = 'https://ai.dev.finto.fi/textract/'//'http://localhost:8001/textract/'; | ||
var textract_base_url = 'https://ai.dev.finto.fi/textract/' | ||
} else { | ||
// local development with VS Code Live Server extension - use APIs of Annif on localhost via Live Server proxy (overcomes CORS error by /v1/detect-language) |
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 we want to have logic like this on the main branch?
(I'm fine either way, just asking whether this is intentional)
}, | ||
body: JSON.stringify({ | ||
text: this.text, | ||
languages: ["fi", "sv", "en"] // TODO Here should be only langs that selected project supports |
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.
Is the TODO a problem in practice for current Finto AI? For some vocabs, we don't support all languages.
.then(response => response.json()) | ||
.then(data => { | ||
this.text_language = data.results[0].language; | ||
// this.text_language_detection_score = data.results[0].score; TODO Add to tooltip |
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.
Is the TODO a problem? Should it be fixed now instead of leaving it like this?
@@ -254,7 +323,9 @@ const mainApp = createApp({ | |||
}) | |||
.then(data => { | |||
this.projects = data.projects | |||
this.selected_project = this.projects[0].project_id | |||
// Assume vocabulary id is a prefix of project id |
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.
Ideally, the vocab ID would be returned by the Annif REST API, but it doesn't expose it currently. I opened an issue about it on the Annif side.
computed: { | ||
disabledLanguages() { | ||
// Map of languages and their enabling criteria based on vocabularyId | ||
return { |
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.
Aren't the duplicate keys here problematic? Does this really work as intended?
Co-authored-by: Osma Suominen <osma.suominen@helsinki.fi>
Co-authored-by: Osma Suominen <osma.suominen@helsinki.fi>
Co-authored-by: Osma Suominen <osma.suominen@helsinki.fi>
This changes the first menu from "Vocabulary and text language" (Annif project) to "Vocabulary" and adds a menu "Text language" (the Annif project is selected as a combination of the selected vocabulary and language). Thus, the text language can be selected independently of the vocabulary; when a vocabulary is selected, that does not have Annif projects for some languages, those language items are disabled in the languages menu.
Also adds automatic language detection, which updates the selection in the language menu. The language detection is run when
This PR branch is based on the branch of PR #18, so it's better to merge that to main before this. Closes #9.