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

Show short name of vocabulary for property values from another vocab #1084

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

osma
Copy link
Member

@osma osma commented Oct 20, 2020

This one-line (plus test) PR will change the vocabulary names that are shown on the concept page next to property values that come from another vocabulary known to Skosmos. This change was requested by the Kanto/finaf project. The long vocabulary names clutter the display of property values. Using the short names instead makes the display a bit cleaner.

Before this PR:

image

After:

image

@osma osma self-assigned this Oct 20, 2020
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented Oct 20, 2020

Codecov Report

Merging #1084 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1084   +/-   ##
=========================================
  Coverage     60.32%   60.32%           
  Complexity     1592     1592           
=========================================
  Files            32       32           
  Lines          4429     4429           
=========================================
  Hits           2672     2672           
  Misses         1757     1757           
Impacted Files Coverage Δ Complexity Δ
model/ConceptPropertyValue.php 87.95% <100.00%> (ø) 47.00 <0.00> (ø)

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 299ee49...04e43bd. Read the comment docs.

@kouralex
Copy link
Contributor

I wonder if this could be implemented as an option, i.e., as a Skosmos property?

Nice work anyway! :)

@osma
Copy link
Member Author

osma commented Oct 27, 2020

@kouralex

I wonder if this could be implemented as an option, i.e., as a Skosmos property?

If there's a genuine need for both long and short names, then making it configurable would make sense. I'm a bit skeptical. I'd prefer just to merge this change as-is, since I think that short vocabulary names are better in any case. They are already used in other contexts in the UI, e.g. search results.

@kouralex
Copy link
Contributor

kouralex commented Oct 27, 2020

@osma

@kouralex

I wonder if this could be implemented as an option, i.e., as a Skosmos property?

If there's a genuine need for both long and short names, then making it configurable would make sense. I'm a bit skeptical. I'd prefer just to merge this change as-is, since I think that short vocabulary names are better in any case. They are already used in other contexts in the UI, e.g. search results.

I see your point. The advantage of your solution is that it is simpler, however, for example YSO does not yell a thing to a Swedish person (as it is ALLFO in their point of view). That said, I am willing to accept your solution, just wanted to point out this case.

@osma
Copy link
Member Author

osma commented Oct 27, 2020

I see your point. The advantage of your solution is that it is simpler, however, for example YSO does not yell a thing to a Swedish person (as it is ALLFO in their point of view). That said, I am willing to accept your solution, just wanted to point out this case.

Short names are (or can be) language specific too, just like long names of vocabularies.

@kouralex
Copy link
Contributor

@osma

Short names are (or can be) language specific too, just like long names of vocabularies.

I stand corrected (it seems like this has been like that, as your said, since v0.6).

I suppose there is no problem then and you have pointed out your preference on this matter. Go on and merge! :)

@osma osma merged commit 5a7175e into master Oct 27, 2020
@osma osma deleted the propval-vocab-use-shortname branch October 27, 2020 13:27
@osma osma added this to the 2.9 milestone Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants