-
Notifications
You must be signed in to change notification settings - Fork 381
IdentifierLinker: Mark the type of link #4800
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
IdentifierLinker: Mark the type of link #4800
Conversation
|
This could be either an alternative to #4774 or an additional mechanism.
|
| 'label' => 'Demonstrating ' . strtoupper($type) . " link for $id with icon " | ||
| . ($icon ?? '[null]'), | ||
| 'localIcon' => $icon, | ||
| 'linkType' => rand(0, 1) ? 'foo' : 'bar', |
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 don't know how 'real' the demo driver output is supposed to be...can improve this if needed.
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 don't think it needs to be very realistic, but maybe calling these foo-link-type and bar-link-type would make the context less ambiguous if we use this for testing. Having too many foo and bar values can make things ambiguous sometimes.
| @@ -1,5 +1,5 @@ | |||
| <?php | |||
| $attribs = ['class' => 'identifierLink', 'data-instance' => $instance ?? 0]; | |||
| $attribs = ['class' => 'identifierLinks', 'data-instance' => $instance ?? 0]; | |||
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 have renamed this wrapper element class to plural identifierLinks because
- It can contain any number of links (0-n) depending on what the data source returns
- I need an actual wrapper element
identifierLinkfor each of those links.
| <div class="identifierLink" | ||
| <?php if (isset($link['linkType'])): ?>data-link-type="<?=$link['linkType']?>" <?php endif; ?> | ||
| > |
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.
Wrapping the <a> in a <div>, we can remove the <br>. And so then hiding any individual identifierLink via CSS doesn't leave the whitespace.
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.
Another option would have been to add the identifierLink class and linkType to the anchor element itself, and change its CSS display property from inline to block. But because it's an icon link it already has a custom display property. Seems safer to leave that alone.
| echo $this->assetManager()->outputInlineScriptLink('identifierLinks.js'); | ||
| ?> | ||
| <span<?=$this->htmlAttributes($attribs)?>></span> | ||
| <div<?=$this->htmlAttributes($attribs)?>></div> |
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 span-to-div change is not strictly necessary, but it was effectively a block-level element to begin with. So as long as I'm making various other potentially breaking changes to the templates and JS, may as well do this one.
demiankatz
left a comment
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.
Thanks, @maccabeelevine -- see below for some comments/suggestions.
I'm not too concerned about the breaking changes since this functionality hasn't been released yet -- but it would be good to wrap this up before the 11.0 release, or else we'll have to worry a little more carefully about BC issues.
| 'label' => 'Demonstrating ' . strtoupper($type) . " link for $id with icon " | ||
| . ($icon ?? '[null]'), | ||
| 'localIcon' => $icon, | ||
| 'linkType' => rand(0, 1) ? 'foo' : 'bar', |
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 don't think it needs to be very realistic, but maybe calling these foo-link-type and bar-link-type would make the context less ambiguous if we use this for testing. Having too many foo and bar values can make things ambiguous sometimes.
Co-authored-by: Demian Katz <[email protected]>
Ok I forgot that IdentifierLinker itself is introduced in 11... this has been a very long release cycle. :) So yeah nothing really breaking here. |
demiankatz
left a comment
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.
Thanks, @maccabeelevine!
Use a data element to mark the type of link provided by the IdentifierLinker. Both BrowZine and Unpaywall provide at least two different types of links. It may be useful to hide (via css) a particular link type on some templates.
I.e. and example of custom CSS to hide BrowZine links that just point back to the link resolver:
TODO