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

EPF: Add holdings list to record view #2975

Merged
merged 4 commits into from Jul 5, 2023

Conversation

maccabeelevine
Copy link
Member

@maccabeelevine maccabeelevine commented Jul 3, 2023

Following up on #2973 and specifically this comment.

Provide a core.phtml for EPF instead of relying on the parent record driver (EDS)'s template.

  • Remove display logic for the many fields which don't apply to EPF.
  • Add a list of full-text holdings.

@maccabeelevine
Copy link
Member Author

@demiankatz This is for discussion purpose only (when you're back from the holiday break). I've implemented the list of holdings two ways, and I'm not sure either is good.

  1. In the main bibliographic details table. This is how EDS lists "HTML Full Text Available" links, so maybe related, although for EDS it's a single link and here it's a list.
  2. Right below that table. This is where EDS displays "Custom Links", which again maybe related, although I think it looks awful.
  3. Not implemented yet, but there could instead be a "Holdings" RecordTab with this info. Looking at the existing holdingsils and holdingswordcat, this is not really like either, and would have to be a third holdingsepf or something.

I'm leaning to options 1 or 3 but wanted your input or another idea altogether. Option 3 seems semantically closest to other record types. Option 1 is appealing only because these links are arguably the thing patrons will be interested in 99% of the time they click onto the record view, so why relegate to a tab. What do you think?

epf-holdings

@demiankatz
Copy link
Member

Thanks, @maccabeelevine. I agree that option 2 is the least desirable. I would be happy to use option 1 as the path of least resistance. If you prefer option 3, though, it may be possible to implement the getURLs method in the EPF record driver and create a derivative of HoldingsILS that does the link display without the actual ILS stuff. Of course, our current link display logic doesn't support having a text description outside of the link itself as EPF does with date ranges, so that might also be another feature to consider adding. All of which clearly says that option 3 is a lot more work than option 1, and reinforces my thought that option 1 is not a bad starting point, though the work required for option 3 could all be useful for other things in the long run and wouldn't be a waste of time.

@maccabeelevine
Copy link
Member Author

@demiankatz Ok, I'm still conflicted on this as well, but I agree that Option 1 is at least a good starting point and a big improvement over what exists now. Can consider Option 3 as a later improvement.

@maccabeelevine maccabeelevine marked this pull request as ready for review July 5, 2023 13:22
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question...

foreach($holdings as $holding):
if (!empty($holding)): ?>
<li>
<a href="<?=$holding['URL']?>"><?=$holding['Name']?></a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these values need to be escaped? I notice they're also not escaped in full-text-links.phtml, which may be something I overlooked in the original review.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely. Fixed.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks, @maccabeelevine!

@maccabeelevine maccabeelevine merged commit 776c5af into vufind-org:dev Jul 5, 2023
7 checks passed
@maccabeelevine maccabeelevine deleted the epf-record branch July 5, 2023 20:59
bpalme pushed a commit to bpalme/vufind that referenced this pull request Aug 5, 2023
Also escape text within full-text-links on the results page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants