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

Display concept types for search #781

Closed
wants to merge 2 commits into from

Conversation

kinow
Copy link
Collaborator

@kinow kinow commented Aug 22, 2018

Add concept types for search results. Icon edited from https://feathericons.com/, but happy to edit the commit and replace by another icon if necessary (due to colour, size, license, etc).

Closes #776

@osma osma added this to the 2.0 milestone Aug 24, 2018
@osma
Copy link
Member

osma commented Aug 24, 2018

Thanks, looks very promising! But a couple of issues:

  1. I tried this with YSO, which has two rdf:types for some concepts, and for some reason one of them shows up empty:

skosmos-type

The other one shows up as skos:Concept, which is not ideal, it would be better to translate it to "Concept" (in English, or the corresponding term in the current UI language).

Replacing {{ concepttype }} with {{ concepttype | trans }} in search-results.twig should handle the translation, but currently the results are a bit funny for the first type which is returned as the empty string, resulting in a translation with POT file metadata. So the empty type problem has to be fixed first, then the translations should work just by adding | trans.

  1. Regarding the icon - the feather icons are nice, and the MIT license they use is of course very liberal, but still using them would add another dependency, i.e. a new source that must be credited. So it would be better to just draw an icon from scratch, or use a CC0 icon (for example one from Iconmonstr.com) that doesn't require attribution.

@osma
Copy link
Member

osma commented Aug 24, 2018

Sorry, I mixed up things, Iconmonstr isn't actually CC0. Their license prohibits e.g. redistribution. So maybe not ideal here.

@kinow
Copy link
Collaborator Author

kinow commented Aug 27, 2018

Will take a look with more calm tomorrow. But a quick analysis today (last thing before going to bed, so might have missed a few things), I've found that:

  • The other concept is actually a NULL, while the one being displayed is a valid string... no idea why the query returned a NULL... I checked the yso.ttl, and the types Concept & Hierarchy are correctly set up.

  • Not displaying skos:Concept and showing the right translation instead sounds like a good idea. But both {% trans concepttype %} and {{ concepttype | trans }} resulted in:

image

Which honestly makes no sense right now 😕

  • The icons from iconmonstr are amazing! So that's where you get the icons from? Bookmarked, and will update the PR again before the weekend.

Thanks!!!

@osma
Copy link
Member

osma commented Aug 28, 2018

See my comment above about iconmonstr - they're not CC0 and thus are not suitable for this. I have used them in another project with different license constraints. The Skosmos icons are custom made, most of them were drawn by the design company Hahmo that created the layouts.

I think it would be best to draw a custom icon. I can get someone to do that if necessary. You can concentrate on the code :)

By the way, the concept page already shows the type of concept (unless it's just skos:Concept). Maybe you can reuse that code?

@kinow
Copy link
Collaborator Author

kinow commented Aug 28, 2018

Hi @osma ,

Sorry, reading again I understood - finally - that I can't use those icons. Such a shame, because they look good IMO! :)

I think it would be best to draw a custom icon. I can get someone to do that if necessary. You can concentrate on the code :)

That'd be great!

By the way, the concept page already shows the type of concept (unless it's just skos:Concept). Maybe you can reuse that code?

Thanks for that! Now I can confirm my suspicion. The query doesn't have the prefix for ysa-meta, so it returns a resource with URI (i.e. instead of ysa-meta:Hierarchy, it's something like http://...ysa-meta/Hierarchy or so).

The code in this pull request worked with skos:Concept by luck. The "events and action" concept is of type skos:Concept and ysa-meta:Hierarchy.

Looks like the concept page has some checks for that already in place. I copied the part that prints the type, and it works out of the box, just with the incorrect layout.

Let me see if I can use the code from that page here then. Great idea @osma (as always).

Thanks!

@kinow kinow force-pushed the display-concepttype-search branch from 829cc5b to de79818 Compare August 29, 2018 09:19
@osma
Copy link
Member

osma commented Aug 29, 2018

Moving to 2.1 since this will take some more work to be ready

@osma osma modified the milestones: 2.0, 2.1 Aug 29, 2018
@kinow
Copy link
Collaborator Author

kinow commented Aug 29, 2018

@osma just updated pull request with a new version, if you'd like to take a look (coincidentally was looking at the issue when your comment popped in)

@kinow
Copy link
Collaborator Author

kinow commented Aug 29, 2018

image

@kinow
Copy link
Collaborator Author

kinow commented Aug 29, 2018

image

@kinow
Copy link
Collaborator Author

kinow commented Aug 29, 2018

Tried to simplify the code from the concept page. Hopefully didn't simplify it too much. It should display the same text from the concepts page.

And rebased the branch too, to make review/merging easier 👍

@kinow
Copy link
Collaborator Author

kinow commented Dec 12, 2018

Oh, I think #827 will supersede this one?

@joelit
Copy link
Contributor

joelit commented Dec 12, 2018

Yeah, I thought it was simpler to create a new PR than to push the changes to your branch.

@joelit joelit closed this Dec 12, 2018
@kinow
Copy link
Collaborator Author

kinow commented Dec 12, 2018

Not a problem for me @joelit , happy to close this one once #827 gets merged :-)

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.

3 participants