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

Add record tab for displaying channels. #4165

Merged
merged 13 commits into from
Jan 27, 2025

Conversation

demiankatz
Copy link
Member

@demiankatz demiankatz commented Dec 16, 2024

This PR creates a tab to embed channels on the record page. It is configured to be disabled by default; uncomment the relevant line(s) in RecordTabs.ini to try it out.

This also adds a missing translation: the text "Channels" is already used in breadcrumbs but was missing from language files.

The ChannelLoader code has also been simplified to take advantage of constructor property promotion.

TODO:

  • Discuss: do we need a way to configure different channels in the tab vs. the normal "record channels" context?
  • Add test coverage
  • Follow-up action (separate PR): deprecate the "similar items" tab in favor of the richer channels tab. We should create a default configuration of the Channels tab that behaves like similar items in order to take its place.

@demiankatz demiankatz added this to the 11.0 milestone Dec 16, 2024
@demiankatz demiankatz added the new language strings adds new text in need of translation label Dec 19, 2024
Copy link
Member

@maccabeelevine maccabeelevine left a comment

Choose a reason for hiding this comment

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

Nice functionality, and the code looks fine. One functional comment below.

Discuss: should we deprecate the "similar items" tab in favor of the richer channels tab? We could create a default configuration of the Channels tab that behaves almost exactly like similar items in order to take its place. (This might make the most sense as a follow-up action after merging this PR).

Yes I think this would be a good follow-up PR. That old carousel is not great.

$request = $this->getRequest() ?: null;
$query = $request?->getQuery();
$driver = $this->getRecordDriver();
$context = ['displaySearchBox' => $this->options['include_channels_search_box'] ?? false];
Copy link
Member

Choose a reason for hiding this comment

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

I agree with this being false by default; I'm having trouble imagining why anyone would want the search box in this context at all, but I guess it's good for discovery of the channels feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I don't really have a use case in mind for why you would want the search box in the tab. I just made the option configurable because it seemed more flexible than hard-coding it as disabled. I'm open to taking away the option if you think it's more confusing to have it than to omit it, though.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, my two cents is just set displaySearchBox to be false. If you or anyone else had a use case I would agree, but otherwise I think it's just another thing to maintain and deal with the ramifications like the wording issue below. Just my $0.02.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've gone ahead and removed it -- we can always restore it in the future if a real use case emerges.

@@ -23,7 +23,7 @@
}
?>

<?php if (empty($token)): ?>
<?php if (empty($token) && ($displaySearchBox ?? true)): ?>
<form action="<?=$this->url('channels-search')?>" class="channel-search form-inline">
<?=$this->transEsc('channel_searchbox_label')?>
Copy link
Member

Choose a reason for hiding this comment

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

If the search box is enabled, which again I'm confused on, I wonder if the label would need to be different in this context. On the one hand, the search box does the exact same thing, so probably not. On the other hand, would the patron be expecting the list of channels to be more record-relevant, and so the label might want to reinforce that? IDK.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could add a config setting to override this, but then we're creating an extra config for a hypothetical situation. Probably best to wait until we have a use case (or, if we decide to take away the search box in tab option entirely, this becomes a non-issue).

@demiankatz demiankatz merged commit e186d90 into vufind-org:dev Jan 27, 2025
6 checks passed
@demiankatz demiankatz deleted the channels-tab branch January 27, 2025 17:58
@demiankatz
Copy link
Member Author

Thanks again, @maccabeelevine -- I've merged this and have started #4209 to eliminate the old carousel tab as we discussed. I'm running into some problems there that I won't have time to fully sort out today, but I'll formally request a review when everything is in proper shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature new language strings adds new text in need of translation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants