-
-
Notifications
You must be signed in to change notification settings - Fork 33
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 a badge containing DOI for peer-reviewed lessons #97
Conversation
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.
This is a good start! There are some tweaks that I want to make because right now, it doesn't really jump out, which was the initial intention of the icon.
I'm not going to request changes from your end, I'm going to make the changes and then merge it next week when I have your approval.
In addition, I am going to rework the stage icons to display as badges instead of icons (which fixes #79) and I will modify the CSS for the main image to have a right margin of 1em so that we no longer have to use the non-breaking spaces.
inst/pkgdown/templates/header.html
Outdated
{{#stable}} | ||
{{#doi}} | ||
<abbr class="badge badge-light" title="This lesson has passed peer review"> | ||
<i aria-hidden="true" class="icon" data-feather="user-check" style="color: #383838; border-radius: 5px"></i> |
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.
this icon element can be embedded within the <a>
element
inst/pkgdown/templates/header.html
Outdated
{{#doi}} | ||
<abbr class="badge badge-light" title="This lesson has passed peer review"> | ||
<i aria-hidden="true" class="icon" data-feather="user-check" style="color: #383838; border-radius: 5px"></i> | ||
<a href="{{doi}}" class="external-link alert-link" style="color: #383838;">DOI: {{doi}}</a> |
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.
the href
attribute should be https://doi.org/{{doi}}
because usually DOIs are displayed without the HTTPS part. This would also make it easier for us to change the DOI URL if the doi.org
domain decides to change.
inst/pkgdown/templates/header.html
Outdated
@@ -32,6 +32,14 @@ | |||
<span class="visually-hidden">This lesson is in the beta phase, which means that it is ready for teaching by instructors outside of the original author team.</span> | |||
</abbr> | |||
{{/beta}} | |||
{{#stable}} | |||
{{#doi}} | |||
<abbr class="badge badge-light" title="This lesson has passed peer review"> |
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.
Did you want to leave the badge as a white box that blends in with the background? I know that we went through the primary colours in Brand identity for the icons, but we may be able to come up with something that looks pleasing enough.
@tobyhodges, in the future, when you add something new to {varnish}, if you can, please provide a screenshot of the changes---it will help with the review. Here are the changes that I made: |
I'm going to go ahead and merge this. I will work with @froggleston to release it next week. |
Resolves #40 and carpentries/workbench#67 by adding a badge with a DOI for lessons that have passed peer review.
(Partnered with carpentries/sandpaper#535)
ZNK note: this will also resolve #79