Migrate LanguageFilter to KMultiSelect - #6093
Conversation
|
👋 Hi @Prashant-thakur77, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
|
@LianaHarris360 I’ve created the PR for the LanguageFilter with KMultiSelect. :) |
8b58fe3 to
3d58905
Compare
|
📢✨ Before we assign a reviewer, we'll turn on |
🟡 Waiting for changesLast updated: 2026-08-20 18:37 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6093 — the migration is faithful: itemText="name" + :searchKeys="['related_names', 'id']" reproduces what the old concatenated item-text matched (KDS 5.9.0 useMultiSelectList filters itemText first, then each searchKeys entry, unwrapping arrays), and the messages map covers every key KMultiSelect's validator requires. Nice side-effect: display and matching are now separate, so a query straddling the old join('') boundary no longer false-matches.
CI passing. Manual QA did not run for this review — the two layout behaviours below are unverified in a browser.
- suggestion — dropdown now renders in place inside the panel's
overflow-y: autocontainer, with no flip logic (LanguageFilter.vue:3) - suggestion — deleted chip-width guard has no KDS equivalent (LanguageFilter.vue:14)
- suggestion — no spec covers the migrated behaviour, unlike #6092 (LanguageFilter.vue:9)
- nitpick ×4 —
diron native names, redundant:multiple, duplicatedmessagesmap, missingcontexton new$trs
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| <template> | ||
|
|
||
| <VAutocomplete | ||
| <KMultiSelect |
There was a problem hiding this comment.
suggestion: The menu moves from a detached overlay into a scrolling ancestor.
VAutocomplete was used bare, so Vuetify's Detachable mixin appended the menu to [data-app]. KDS KMultiSelectDropdown renders in place (position: absolute; top: 100%; max-height: 256px) and useMultiSelectDropdown.js has no viewport/flip logic — no getBoundingClientRect, no top branch. Its ancestor clips: CatalogFilterPanelContent.vue .filters-container { min-height: 0; overflow-y: auto; }.
This is what shared/views/form/DropdownWrapper.vue exists for — the sibling MultiSelect and CountryField in the same panel use it to flip upward when there isn't 300px below; KMultiSelect can't.
Manual QA didn't run here, so this is unconfirmed. The field is 2nd in the panel so tall desktop viewports likely clear it. Worth checking: short viewport (~600–700px) with the panel scrolled so the field sits in the lower 256px, and windowIsSmall where the panel renders inside SidePanelModal. If it does clip, it belongs upstream in KDS.
| </VAutocomplete> | ||
| :noResultsText="$tr('noMatchingLanguageText')" | ||
| :messages="messages" | ||
| /> |
There was a problem hiding this comment.
suggestion: The deleted style block carried its own reason — "Need to set otherwise chips will exceed width of selection box" — plus max-width: 100% on chip content and a 250px truncate on option rows.
KDS 5.9.0 reintroduces that condition: KChip is white-space: nowrap with no max-width, .kmselect-input is flex-wrap: wrap with no overflow/min-width: 0, and KListboxOption doesn't truncate. Longest native names in window.publicLanguages (português brasileiro, Bahasa Indonesia, српски језик) in the narrow desktop sidebar are the case to look at.
The #chip/#option slots are the hook if it overflows. Note the old markup wrapped both chip and option in a VTooltip showing the full name — that was the readability affordance for the truncation, and it was hover-only, so prefer wrapping over reintroducing truncate+tooltip.
| multiple | ||
| itemValue="id" | ||
| itemText="name" | ||
| :searchKeys="['related_names', 'id']" |
There was a problem hiding this comment.
suggestion: Nothing tests the behaviour this line is responsible for. CatalogFilterPanelContent.spec.js:82 only asserts the Languages label renders.
b63db298c (#6092), the same migration one commit earlier, added contentNodeFields/__tests__/categoryOptions.spec.js covering selection, clear, and the clear-all button by role. Those three apply here, plus the one behaviour unique to this component: typing an English name or a language code still matches. A KDS bump or prop rename would otherwise break search silently.
publicLanguages is built at module load from window.publicLanguages, so the spec has to set that global before importing the component.
| box | ||
| multiple | ||
| itemValue="id" | ||
| itemText="name" |
There was a problem hiding this comment.
nitpick: itemText="name" resolves to native_name — content text in its own script, so Arabic/Hebrew/Farsi entries are RTL inside an LTR app. Neither KMultiSelectNode nor KChip sets dir (KDS 5.9.0); shared/views/form/MultiSelect.vue:34 sets dir="auto" on its option text.
Asking rather than asserting, because the old code deliberately went the other way — :labelDir="null" suppressed KCheckbox's dir="auto". If that was working around the checkbox row specifically, the #option/#chip slots would let you scope dir="auto" to the text span.
| itemValue="id" | ||
| itemText="name" | ||
| :searchKeys="['related_names', 'id']" | ||
| :multiple="true" |
There was a problem hiding this comment.
nitpick: multiple defaults to true (KMultiSelect/index.vue:586); CategoryOptions omits it.
| return; | ||
| } | ||
| firstItem.$el.scrollIntoView(); | ||
| messages() { |
There was a problem hiding this comment.
nitpick: Near-verbatim copy of CategoryOptions.vue:200-214 — same two translators, only itemsSelected and cleared differ. Two is under the Rule of Three, so leaving it is right; flagging for whoever migrates the third (MultiSelect.vue / CountryField.vue), at which point a shared/strings/ helper taking the two component-specific messages earns its keep.
| $trs: { | ||
| languageLabel: 'Languages', | ||
| noMatchingLanguageText: 'No language matches the search', | ||
| itemsSelected: '{count, plural, one {# language selected} other {# languages selected}}', |
There was a problem hiding this comment.
nitpick: Both new strings are screen-reader announcements with no context — "Cleared # selections" is hard to translate without knowing what's cleared. No $trs block in channelList/ carries context today, so this isn't the current convention; commonStrings.js, where the neighbouring messages keys come from, does.
2 participants
Summary
Migrates
LanguageFilterfrom Vuetify'sVAutocompleteto KDSKMultiSelect, as part of moving Studio off Vuetify.KMultiSelectrenders its own chips and options, so the hand-rolled#selectionand#itemslot templates, theStudioChipimport, the search helper, the scroll-reset method, and the Vuetify-specific styles all go away.Partof: learningequality/kolibri-design-system#1259
Notable points:
item-textreturningname + related_names + id, then overrode both slots to display onlynamethe concatenation existed purely for filtering. That's now expressed directly asitemText="name"for display plus:searchKeys="['related_names', 'id']"for matching,so searching by native name, English name, or language code all still work.
StudioChipis no longer needed here becauseKMultiSelectrenders chips itself usingKChip, which is the same component upstreamed into KDS (identical markup and styles).No spec file existed for this component and none is added; behavior was verified as described below.References
unstablevia Upgrade KDS to 5.9.0 #6090.Reviewer guidance
Check for no regrressions then before and proper refractoring.
Screencast.From.2026-08-17.02-43-29.mp4
AI usage
I used Claude to write this description and made the changes as needed.