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

The vocabulary list on the frontpage shows the vocabulary names in th… #1259

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

Conversation

miguelvaara
Copy link
Contributor

…e default language of the vocabulary

Thanks @henriyli, @osma and @kouralex for your contribution :-)

Reasons for creating this PR

To follow wishes mentioned in the PR #747

Link to relevant issue(s), if any

#731

  • Closes #

Description of the changes in this PR

The vocabulary list on the front page shows the vocabulary names in the default language of the vocabulary

Known problems or uncertainties in this PR

Checklist

  1. For the vocabulary of your choice, point out the default language in the config.ttl (make sure the dc:title is also defined with respect to the default language)
  2. In Skosmos, select the content language not used in the vocabulary
  3. On the Skosmos front page check if the vocabulary name in the list is shown in the default language

When it comes to the phpUnit tests, at the moment I am investigating ways to run PHPUnit tests for Symphony Twig templates. Tests are coming a bit later.

  • phpUnit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if not, explain why below)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@miguelvaara miguelvaara requested a review from osma December 22, 2021 17:02
@codecov
Copy link

codecov bot commented Dec 22, 2021

Codecov Report

Merging #1259 (7d99329) into master (a770d4c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1259   +/-   ##
=========================================
  Coverage     69.26%   69.26%           
  Complexity     1646     1646           
=========================================
  Files            32       32           
  Lines          4041     4041           
=========================================
  Hits           2799     2799           
  Misses         1242     1242           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a770d4c...7d99329. Read the comment docs.

Comment on lines 18 to 25
{% set useDefaultLanguage = null %}
{% if request.contentLang not in vocab.config.languages and request.contentLang != '' %}
{% set useDefaultLanguage = true %}
{% endif %}
<li><a class="navigation-font" href="{{ vocab.id }}/{{ request.lang }}/{% if request.contentLang not in vocab.config.languages and request.contentLang != '' %}?clang={{ vocab.config.defaultLanguage }}{% endif %}">
{% if useDefaultLanguage is not null %} {{ vocab.title(vocab.config.defaultLanguage) }}
{% else %} {{ vocab.title(request.contentLang) }}
{% endif %} </a></li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we use something like (not tested) this?

Suggested change
{% set useDefaultLanguage = null %}
{% if request.contentLang not in vocab.config.languages and request.contentLang != '' %}
{% set useDefaultLanguage = true %}
{% endif %}
<li><a class="navigation-font" href="{{ vocab.id }}/{{ request.lang }}/{% if request.contentLang not in vocab.config.languages and request.contentLang != '' %}?clang={{ vocab.config.defaultLanguage }}{% endif %}">
{% if useDefaultLanguage is not null %} {{ vocab.title(vocab.config.defaultLanguage) }}
{% else %} {{ vocab.title(request.contentLang) }}
{% endif %} </a></li>
{% set useDefaultLanguage = request.contentLang not in vocab.config.languages and request.contentLang != '' %}
<li><a class="navigation-font" href="{{ vocab.id }}/{{ request.lang }}/{% if useDefaultLanguage %}?clang={{ vocab.config.defaultLanguage }}{% endif %}">
{% if useDefaultLanguage %} {{ vocab.title(vocab.config.defaultLanguage) }}
{% else %} {{ vocab.title(request.contentLang) }}
{% endif %} </a></li>

I think we have the same condition in two places, so maybe storing the true or false value of that condition in that variable would simplify it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way you proposed was clearly more elegant, I changed the code. Thanks a lot @kinow

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@miguelvaara
Copy link
Contributor Author

Another question is where the business logic is located. For instance, I think it is also possible to modify a request according to the needs mentioned in the original issue. The configuration and therefore the default language of the vocabulary could be checked before accessing the data depending on the request.

@@ -24,3 +30,8 @@
</div>
{% endif %}
</div>

Copy link
Member

Choose a reason for hiding this comment

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

I guess the newlines are not needed?

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.

3 participants