Adds new fulfillment links and adjusts link logic #329
Conversation
Pull Request Test Coverage Report for Build 20755458632Details
💛 - Coveralls |
JPrevost
force-pushed
the
use-284-more-libkey
branch
from
16732b1 to
a430f8e
Compare
JPrevost
changed the title
| @include buttonSecondary; | ||
| } | ||
|
|
||
| // Treat libkey links similarly to the primo links |
There was a problem hiding this comment.
@djanelle-mit There may be a better way to avoid some duplication here that I am introducing, but I'm going to just point at it and make it your problem ;)
There was a problem hiding this comment.
The styles seem correct from my general inspection, but Dave is the better judge of this than I am - so I'm deferring to him for the close reading.
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
None of my comments are blocking, so I'm approving as-is.
One of my comments is a general mental alignment question, while the other two comments (about a test, and the openurl_link element in the libkey model) are more detailed.
| browzine_link: browzine_link(external_data), | ||
| html_link: html_link(external_data), | ||
| pdf_link: pdf_link(external_data), | ||
| openurl_link: openurl_link(external_data) |
There was a problem hiding this comment.
Not sure if this is blocking:
Does this openurl link get shown in the UI at the moment? I don't see it being added to the lookup partial. I'm fine with leaving it in if there's a plan to do something with it down the road, but I want to make sure I'm not missing logic somewhere that would display it now. If that's supposed to exist, I've missed it.
There was a problem hiding this comment.
You are correct in that it does not get used. My gut feeling is the logic is not done yet so I prepped by adding this rather than having to remember later that we could be pulling it but aren't.
| @@ -1,3 +1,20 @@ | |||
| <% if Libkey.enabled? && @libkey.present? %> | |||
| <%= link_to( @libkey[:text], @libkey[:link], class: 'button libkey-link' ) %> | |||
| <%if @libkey[:pdf_link].present? || @libkey[:html_link].present? %> | |||
There was a problem hiding this comment.
Non-blocking, more for my own mental alignment than this PR:
Am I overthinking things if I hope for the view layer to just have an iterator over "show all the links in the @libkey object`, with the conditional logic of this or that link being kept in the model?
To be clear, I'm not asking for this template to change - just reflecting on what I've been coming to think of as the ideal state for a view layer versus a model, and principles like "thin controller, fat model". Making sure I keep our ideal state in mind will help me make sure whatever PRs I ship are hopefully in line with what everyone else is also hoping for.
There was a problem hiding this comment.
I hear you...I'm open to revisiting this in the future but I suspect a fair bit of churn in the fulfillment links for a bit and didn't want to try to optimize anything before we know what the actual target is.
| @include buttonSecondary; | ||
| } | ||
|
|
||
| // Treat libkey links similarly to the primo links |
There was a problem hiding this comment.
The styles seem correct from my general inspection, but Dave is the better judge of this than I am - so I'm deferring to him for the close reading.
The OpenURL links generated by Primo lead to a page in the Primo UI that does not have the same context as the Primo UI full record. These links also don't always work and lead to a blank white page in Primo UI in some edge cases. As such, we are removing these entirely. For now, there will be no "button" link for some Articles that previously would have them. UX believes users will click the title to see access options. If testing or analytics show this is not the case, we can experiement with adding a button with the same link as is in the title in situations we previously would have had an OpenURL link. https://mitlibraries.atlassian.net/browse/USE-284
Updates LibKey model to include additional link types provided by LibKey: - PDF Link - HTML Link - Browzine Link - OpenURL Link Adjusts BestIntegratorLink to skip "Access options" links per UX request. Updates views to prioritize links. If we have PDF and/or HTML links we display these. If we do not have either, we display the BestIntegratorLink (which no no longer includes "Access options" links). We always display Browzine links if available. Adjusts CSS to use same styling as Primo provided links. https://mitlibraries.atlassian.net/browse/USE-284
JPrevost
force-pushed
the
use-284-more-libkey
branch
from
623b516 to
430fe75
Compare
Please see individual commit messages and ticket for details.
Summary of changes:
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing