Conversation
35e5d09 to
855a9bd
Compare
| const urlParams = this.formatSearchUrlParams() | ||
| if (this.selectedLanguage !== window.SKOSMOS.lang) { // add content language parameter | ||
| urlParams.append('clang', this.selectedLanguage) | ||
| result.pageUrl = window.SKOSMOS.baseHref + result.vocab + '/' + window.SKOSMOS.lang + '/page/?' |
There was a problem hiding this comment.
I think this should rather be done using the getConceptUrl method defined in get-concept-url.js. This is used by the sidebar components to generate concept page links. It's better to have this logic in just one place, and the function will also use a short form URL (/page/p12345) when possible instead of always using the ?uri= form.
The same applies to the vocab-search component.
There was a problem hiding this comment.
Good call - I made an issue (#1856 ) of the use of getConceptUrl in forming links for the concepts. I found out it leaves out the /Skosmos/ installation folder when forming concept links and it doesn't handle the content language too well, i.e. http://localhost/fi/page/?uri=http%3A%2F%2Fwww.yso.fi%2Fonto%2Fyso%2Fp19378&clang= so it needs to be polished a bit, and tested accross all the different use cases, adding tests etc.
Then it should be used here too.
osma
left a comment
There was a problem hiding this comment.
LGTM. Seems to be an improvement overall.
I think updateSearchLang can now be deleted, see comment. Other than that, good for merging.
|
Reasons for creating this PR
Some unimplemented features were spotted in the global search bar. This addessess some of those.
Link to relevant issue(s), if any
Description of the changes in this PR
Known problems or uncertainties in this PR
Checklist
.sr-onlyclass, color contrast)