-
Notifications
You must be signed in to change notification settings - Fork 205
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
fix: fix bioportal link generation and remove redundant links #2655
Conversation
…tal browser links
Two other notes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its ok, but maybe better to not make edits to the specific metadata files without debate, as this decreases trust.
I will wave this through immediately with you just drop the md file changes..
Is there a way to avoid changes to the metadata, but instead removing the code that parses the button from the md file? |
Sure thing. Made this change here: OBOFoundry.github.io/_layouts/ontology_detail.html Lines 73 to 78 in c6e650d
|
<a href="https://bioportal.bioontology.org/ontologies/{{ bioportal_id }}" class="btn btn-outline-primary" target="_blank" rel="noopener"> | ||
BioPortal | ||
</a> | ||
{% endunless %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait the way you do it, if a bioportal link is manually set, wont they now see no button at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't believe so --
Lines 73 through 78 set a local skip_bioportal
variable. If it's false, then lines 80-110 render a BioPortal button, but if it's true, then that's skipped. Regardless, 111-115 render custom buttons for each set of values defined in the ontology's front matter.
OBOFoundry.github.io/_layouts/ontology_detail.html
Lines 111 to 115 in c6e650d
{% for b in page.browsers %} | |
<a href="{{b.url}}" class="btn btn-outline-primary" target="_blank" rel="noopener"> | |
{{b.label}} | |
</a> | |
{% endfor %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks @jsstevenson!
Two issues
This PR removes redundant metadata from the latter and performs a basic if/else "mapping" for edge cases directly in the layout rather than shipping it out to a custom filter.