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

Add HTML classes for properties so they can be targeted in JS and CSS #1046

Merged
merged 2 commits into from
Sep 4, 2020

Conversation

osma
Copy link
Member

@osma osma commented Aug 31, 2020

Fixes #1034

All the div blocks which represent properties will get a HTML class property.

Additionally, those divs will get a specific class identifying the property in question. Usually, the class identifier is generated from the property URI:

  1. For known property URIs that can be shortened, the class will be e.g. prop-dc_identifier or prop-skos_broader
  2. For unknown URI namespaces that cannot be shortened, the class will be derived from the full URI, e.g. prop-http___rdaregistry_info_Elements_a_P50008 (for rdaa:P50008 - but the rdaa prefix is not known by Skosmos)

There are a few special cases for some div blocks representing special purpose properties:

  • preferred label property: prop-preflabel
  • other languages property: prop-other-languages
  • URI "property": prop-uri
  • mapping properties: prop-mapping

Any comments @CaptSolo @kinow?

@osma osma added this to the 2.8 milestone Aug 31, 2020
@codecov
Copy link

codecov bot commented Aug 31, 2020

Codecov Report

Merging #1046 into master will increase coverage by 1.50%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1046      +/-   ##
============================================
+ Coverage     60.32%   61.83%   +1.50%     
- Complexity     1593     1782     +189     
============================================
  Files            32       32              
  Lines          4429     4656     +227     
============================================
+ Hits           2672     2879     +207     
- Misses         1757     1777      +20     
Impacted Files Coverage Δ Complexity Δ
model/ConceptProperty.php 96.42% <100.00%> (+0.13%) 20.00 <1.00> (+1.00)
model/Concept.php 83.80% <0.00%> (+3.99%) 386.00% <0.00%> (+188.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 37601f9...31619f1. Read the comment docs.

Copy link
Collaborator

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Looks good to me! Great improvement @osma!

@osma osma force-pushed the issue1034-html-class-for-properties branch from 0c744b6 to 31619f1 Compare September 2, 2020 06:53
@sonarcloud
Copy link

sonarcloud bot commented Sep 2, 2020

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@kouralex kouralex left a comment

Choose a reason for hiding this comment

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

Very needed improvement indeed! Thanks @osma for this PR.

I just want to explicitly state that

  1. The simplistic way of just adding the prop-{{property.ID}} to the class name will only show the most accurate property ID due to the de-duplication procedure, e.g., on https://finto.fi/yso-paikat/fi/page/?uri=http://www.yso.fi/onto/yso/p507412 you will only have isothes_broaderPartitive and no skos_broader. Some implementation could, in theory, only search for the known skos:broader property but maybe that is a minor detail and may be shrugged off (via YAGNI principle).

  2. The names used for prefixes in config.ttl will end up into the class names, which may cause unexpected values to be used (from the perspective of a third party). This cannot be countered as there is no prefix mappings given.

But it is good as it now, I will happily approve!

@osma
Copy link
Member Author

osma commented Sep 4, 2020

Thanks @kouralex - I'm aware of those edge cases but I think it's worth merging this as-is and then improve later on if it turns out to be necessary.

@osma osma merged commit c72dad5 into master Sep 4, 2020
@osma osma deleted the issue1034-html-class-for-properties branch September 4, 2020 06:17
@CaptSolo
Copy link

CaptSolo commented Sep 8, 2020

Thank you - works like a charm! :-)

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.

Adding HTML IDs / classes to the property value list
4 participants