-
Notifications
You must be signed in to change notification settings - Fork 713
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
Move useChannels & SearchChips to kolibri-common #12745
Move useChannels & SearchChips to kolibri-common #12745
Conversation
Build Artifacts
|
|
||
export default { | ||
name: 'SearchChips', | ||
mixins: [commonCoreStrings], | ||
setup() { | ||
const { languagesMap } = useLanguages(); | ||
const { availableLanguages } = injectBaseSearch(); | ||
const languagesMap = availableLanguages.reduce((map, lang) => { |
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.
Ah yes, this is confirmed by @radinamatic's testing - this needs to either get
the availableLanguages
or access the .value
.
c72320f
to
a48f10b
Compare
removes useLanguageses altogether as they are going unused
a48f10b
to
5a9ea16
Compare
@radinamatic sorry for the delay here - I've fixed the issue noted and the language chips show up correctly now. |
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.
Not seeing any code concerns in here, all useful cleanup.
LGTM now @radinamatic - the issue reported by you is fixed now and there are no regressions caused by this PR. |
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.
Code checks out, QA checks out. This is good to merge.
Thank you @pcenov! 👍🏽 @nucleogenesis I did notice one thing while testing over the weekend, and I wanted to investigate more but then the ⛈️ happened: However, it does not clear the applied filters for any of the dropdown options (Language, Level, Channel, Accessibility). Search results are cleared, but the previously applied filter apparently stays visible, but not actually applied anymore, and the user needs to clear them manually. This can be confusing and we should force them cleared as for the other filtering options in the sidebar. clear-dropdown.mp4 |
Summary
SearchChips
intokolibri-common
w/ other Search componentsSearchChips
gets languages frominjectBaseSearch#availableLanguages
instead ofuseLanguages
useChannels
(and mocks) intokolibri-common
- this simplified theSearchChips
move as it depends onuseChannels
anduseChannels
seems well suited for a move to the common packageuseLanguages
es and all references to itReferences
Closes #12720
Reviewer guidance
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)