-
Notifications
You must be signed in to change notification settings - Fork 691
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
Remove channels label fetching in useBaseSearch composable #12877
Remove channels label fetching in useBaseSearch composable #12877
Conversation
Build Artifacts
|
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 smoke tested search to be sure & the code changes LGTM -- thanks Liana!
@@ -443,14 +410,14 @@ describe(`useSearch`, () => { | |||
it('should not remove any other filters', () => { | |||
const { removeFilterTag, router } = prep({ | |||
categories: 'test1,test2', | |||
channels: 'channel1', | |||
learning_activities: 'watch', |
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.
Curious why specifically watch
here?
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.
There's no specific reason other than seeing it being used higher up in this testing file and using it to replace the filter that was previously there.
Summary
This pull request supports the removal of the 'channels' label from
SearchFiltersPanel
and the corresponding label processing for the 'channels' option used in theuseBaseSearch
composable, in addition to updating the relevant tests.References
Closes #12841
Reviewer guidance
Confirm that
useBaseSearch
no longer exposesavailableChannels
and doesn't handle anything related to the 'channels' label.useBaseSearch
should still filter search results based on the channel the user is currently in.