-
-
Notifications
You must be signed in to change notification settings - Fork 303
Migrate LanguageFilter to KMultiSelect #6093
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
base: unstable
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,73 +1,27 @@ | ||
| <template> | ||
|
|
||
| <VAutocomplete | ||
| <KMultiSelect | ||
| v-model="languages" | ||
| :items="availableLanguages" | ||
| :options="availableLanguages" | ||
| :label="$tr('languageLabel')" | ||
| color="primary" | ||
| item-value="id" | ||
| :item-text="languageSearchValue" | ||
| autoSelectFirst | ||
| :no-data-text="$tr('noMatchingLanguageText')" | ||
| box | ||
| multiple | ||
| itemValue="id" | ||
| itemText="name" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: Asking rather than asserting, because the old code deliberately went the other way — |
||
| :searchKeys="['related_names', 'id']" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Nothing tests the behaviour this line is responsible for.
|
||
| :multiple="true" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: |
||
| clearable | ||
| :search-input.sync="languageInput" | ||
| v-bind="$attrs" | ||
| @change="languageInput = ''" | ||
| @blur="resetScroll" | ||
| > | ||
| <template #selection="{ item }"> | ||
| <VTooltip | ||
| bottom | ||
| lazy | ||
| > | ||
| <template> | ||
| <StudioChip class="ma-1"> | ||
| <div class="text-truncate"> | ||
| {{ item.name }} | ||
| </div> | ||
| </StudioChip> | ||
| </template> | ||
| <span>{{ item.name }}</span> | ||
| </VTooltip> | ||
| </template> | ||
| <template #item="{ item }"> | ||
| <KCheckbox | ||
| :key="item.id" | ||
| :ref="'checkbox-' + item.id" | ||
| v-model="languages" | ||
| :presentational="true" | ||
| :value="item.id" | ||
| class="mb-0 mt-1 scroll-margin" | ||
| :labelDir="null" | ||
| > | ||
| <VTooltip | ||
| bottom | ||
| lazy | ||
| > | ||
| <template #activator="{ on }"> | ||
| <div | ||
| class="text-truncate" | ||
| style="width: 250px" | ||
| v-on="on" | ||
| > | ||
| {{ item.name }} | ||
| </div> | ||
| </template> | ||
| <span>{{ item.name }}</span> | ||
| </VTooltip> | ||
| </KCheckbox> | ||
| </template> | ||
| </VAutocomplete> | ||
| :noResultsText="$tr('noMatchingLanguageText')" | ||
| :messages="messages" | ||
| /> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: The deleted style block carried its own reason — "Need to set otherwise chips will exceed width of selection box" — plus KDS 5.9.0 reintroduces that condition: The |
||
|
|
||
| </template> | ||
|
|
||
|
|
||
| <script> | ||
|
|
||
| import KMultiSelect from 'kolibri-design-system/lib/candidate/multiselect/KMultiSelect'; | ||
| import LanguagesMap, { LanguagesList } from 'shared/leUtils/Languages'; | ||
| import StudioChip from 'shared/views/StudioChip.vue'; | ||
| import { commonStrings } from 'shared/strings/commonStrings'; | ||
| import { communityChannelsStrings } from 'shared/strings/communityChannelsStrings'; | ||
|
|
||
| const publicLanguages = Object.entries(window.publicLanguages || {}).map(([langId, count]) => { | ||
| const baseLanguage = LanguagesMap.get(langId); | ||
|
|
@@ -84,7 +38,7 @@ | |
| export default { | ||
| name: 'LanguageFilter', | ||
| components: { | ||
| StudioChip, | ||
| KMultiSelect, | ||
| }, | ||
| props: { | ||
| value: { | ||
|
|
@@ -96,7 +50,6 @@ | |
| }, | ||
| data() { | ||
| return { | ||
| languageInput: '', | ||
| availableLanguages: publicLanguages, | ||
| }; | ||
| }, | ||
|
|
@@ -109,48 +62,39 @@ | |
| this.$emit('input', value.filter(Boolean)); | ||
| }, | ||
| }, | ||
| }, | ||
| methods: { | ||
| languageSearchValue(item) { | ||
| return item.name + (item.related_names || []).join('') + item.id; | ||
| }, | ||
| resetScroll() { | ||
| const [{ id: firstLangId } = {}] = publicLanguages; | ||
| if (!firstLangId) { | ||
| return; | ||
| } | ||
| const firstItem = this.$refs[`checkbox-${firstLangId}`]; | ||
| if (!firstItem) { | ||
| return; | ||
| } | ||
| firstItem.$el.scrollIntoView(); | ||
| messages() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: Near-verbatim copy of |
||
| const { | ||
| openMenuAction$, | ||
| closeMenuAction$, | ||
| optionsClickableLabel$, | ||
| allOptionsSelectedLabel$, | ||
| allOptionsDeselectedLabel$, | ||
| optionDeselectedLabel$, | ||
| optionSelectedLabel$, | ||
| optionRemovedLabel$, | ||
| } = commonStrings; | ||
| const { clearAllAction$ } = communityChannelsStrings; | ||
| return { | ||
| clearText: clearAllAction$, | ||
| open: openMenuAction$, | ||
| close: closeMenuAction$, | ||
| clickable: optionsClickableLabel$, | ||
| allOptionsSelected: allOptionsSelectedLabel$, | ||
| allOptionsDeselected: allOptionsDeselectedLabel$, | ||
| optionDeselected: optionDeselectedLabel$, | ||
| itemsSelected: ({ count }) => this.$tr('itemsSelected', { count }), | ||
| selected: optionSelectedLabel$, | ||
| removed: optionRemovedLabel$, | ||
| cleared: ({ count }) => this.$tr('selectionsCleared', { count }), | ||
| }; | ||
| }, | ||
| }, | ||
| $trs: { | ||
| languageLabel: 'Languages', | ||
| noMatchingLanguageText: 'No language matches the search', | ||
| itemsSelected: '{count, plural, one {# language selected} other {# languages selected}}', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: Both new strings are screen-reader announcements with no |
||
| selectionsCleared: '{count, plural, one {Cleared # selection} other {Cleared # selections}}', | ||
| }, | ||
| }; | ||
|
|
||
| </script> | ||
|
|
||
|
|
||
| <style lang="scss" scoped> | ||
|
|
||
| // Need to set otherwise chips will exceed width of selection box | ||
| ::v-deep .v-select__selections { | ||
| width: calc(100% - 48px); | ||
| } | ||
|
|
||
| .v-chip, | ||
| ::v-deep .v-chip__content, | ||
| .text-truncate { | ||
| max-width: 100%; | ||
| } | ||
|
|
||
| .scroll-margin { | ||
| /* Fixes scroll position on reset scroll */ | ||
| scroll-margin: 16px; | ||
| } | ||
|
|
||
| </style> | ||
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.
suggestion: The menu moves from a detached overlay into a scrolling ancestor.
VAutocompletewas used bare, so Vuetify'sDetachablemixin appended the menu to[data-app]. KDSKMultiSelectDropdownrenders in place (position: absolute; top: 100%; max-height: 256px) anduseMultiSelectDropdown.jshas no viewport/flip logic — nogetBoundingClientRect, notopbranch. Its ancestor clips:CatalogFilterPanelContent.vue.filters-container { min-height: 0; overflow-y: auto; }.This is what
shared/views/form/DropdownWrapper.vueexists for — the siblingMultiSelectandCountryFieldin the same panel use it to flip upward when there isn't 300px below;KMultiSelectcan'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
windowIsSmallwhere the panel renders insideSidePanelModal. If it does clip, it belongs upstream in KDS.