Skip to content

Replace CategoryOptions expanded mode VAutocomplete with KMultiSelect #6355

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.

Sign up for GitHub

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

Closed
sarwar-jahin-port wants to merge 6 commits into from
Closed

Replace CategoryOptions expanded mode VAutocomplete with KMultiSelect #6355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eac372f
Migrate CategoryOptions expanded mode to KMultiSelect
sarwar-jahin-port Oct 28, 2026
b40bd7b
Fix clear-all and parent removal dropping partial categories
sarwar-jahin-port Oct 28, 2026
c3fe15b
Kept cleared pure and moved the clear-all handling into onKMultiSelec…
sarwar-jahin-port Oct 29, 2026
f343577
Restore cleared-based reset for Clear-all
sarwar-jahin-port Oct 29, 2026
214d6b7
Simplify CategoryOptions back to the plain rebuild
sarwar-jahin-port Nov 1, 2026
c3e7bbc
Remove tests redundant with the simplified contract
sarwar-jahin-port Nov 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 8 additions & 13 deletions ...tion/frontend/channelEdit/components/QuickEditModal/__tests__/EditBooleanMapModal.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,26 +26,21 @@ Object.entries(Categories).forEach(([key, value]) => {
categoriesLookup[newKey] = value;
});

const getOptionCheckboxes = wrapper => wrapper.findAllComponents({ name: 'KListboxOption' });

const getOptionsValues = wrapper => {
const categories = {};
const checkboxes = wrapper.findAllComponents('[data-test="option-checkbox"]');
checkboxes.wrappers.forEach(checkbox => {
const { label, checked } = checkbox.props();
let value;
if (checked) {
value = CheckboxValue.CHECKED;
} else {
value = CheckboxValue.UNCHECKED;
}
categories[categoriesLookup[label]] = value;
getOptionCheckboxes(wrapper).wrappers.forEach(option => {
const label = option.props('label');
const checked = option.attributes('aria-selected') === 'true';
categories[categoriesLookup[label]] = checked ? CheckboxValue.CHECKED : CheckboxValue.UNCHECKED;
});
return categories;
};

const findOptionCheckbox = (wrapper, category) => {
const checkboxes = wrapper.findAllComponents('[data-test="option-checkbox"]');
return checkboxes.wrappers.find(checkbox => {
const label = checkbox.props('label');
return getOptionCheckboxes(wrapper).wrappers.find(option => {
const label = option.props('label');
return categoriesLookup[label] === category;
});
};
Expand Down
173 changes: 2 additions & 171 deletions contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<div>
<KMultiSelect
v-if="!expanded"
:expanded="expanded"

@akolson akolson Nov 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: Clicking a parent category with a selected child now produces a different result in expanded mode. For example, with value: { [DANCE]: [n1] }, clicking School behaves differently:

  • On base: School appears checked because one of its children is selected. Clicking it removes School and all its descendants, so the component emits {} (base isCheckboxSelectedByChildren and onChange).
  • With this PR: School appears indeterminate. Clicking it adds School to the selection, so the component emits { [DANCE], [SCHOOL] }.

Issue #6346 states that "Selecting a parent category gives the same result as it does now." It also requires both modes to share the same component. These two criteria conflict in this case.

@LianaHarris360, which behaviour should we preserve? If the result should match dropdown mode, the acceptance criterion needs to be updated to reflect that. If the existing behaviour must be preserved, KMultiSelect cannot currently support it, so KDS will need a change first.

@sarwar-jahin-port sarwar-jahin-port Nov 3, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

agreed this is a real conflict between the two acceptance criteria, not something I should resolve unilaterally. Leaving that for @LianaHarris360's call happy to implement either direction once it's settled.

@LianaHarris360 LianaHarris360 Nov 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The preference would be to keep the current behavior, but I remember discussing this during the design of KMultiSelect and we ultimately decided that a parent value should only be shown as checked when all of its children are selected.

However when KMultiSelect is in Automatic parent selection mode, the parent value is automatically included in the selected list if any child is selected, and when clicked, it and all of its children are removed from the list.

@akolson akolson Nov 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @LianaHarris360. I checked autoSelectParent in KDS 5.10.0, and I think it does not give the current behavior here, for two reasons. I ran both cases locally with the prop turned on.

  • It adds parents only when a child is selected in the same session (useMultiSelectCascade.js#L176-L187). A resource that already has Dance still shows School as indeterminate, and a click on School adds it.
  • It saves the parents. Selecting Dance emits School, Arts and Dance, and handleSave saves all three on the resource. Currently, only Dance is saved, in both modes.

So I suggest that we keep the KMultiSelect behavior, which is the same as dropdown mode, and change the criterion to "Selecting a parent category gives the same result in both modes." Is that OK with you? If we need parents saved, I think that needs its own issue, because it changes the data for both modes.

@LianaHarris360 LianaHarris360 Nov 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep, that's fine with me. Thanks for looking into it!

:value="autocompleteValues"
:options="categoriesList"
itemValue="value"
Expand Down Expand Up @@ -32,74 +32,6 @@
/>
</template>
</KMultiSelect>

<DropdownWrapper v-if="expanded">
<template #default="{ attach, menuProps }">
<VAutocomplete
:value="autocompleteValues"
:items="categoriesList"
:searchInput.sync="categoryText"
:label="translateMetadataString('category')"
box
clearable
chips
deletableChips
multiple
item-value="value"
item-text="text"
:menu-props="{
...menuProps,
zIndex: 4,
height: 0,
maxHeight: 0,
}"
:attach="attach"
@click:clear="$nextTick(() => removeAll())"
>
<template #selection="data">
<VTooltip
bottom
lazy
>
<template #activator="{ on, attrs }">
<VChip
v-bind="attrs"
:close="!data.item.undeletable"
v-on="on"
@input="remove(data.item.value)"
>
{{ data.item.text }}
</VChip>
</template>
<div>
<div>{{ tooltipText(data.item.value) }}</div>
</div>
</VTooltip>
</template>
</VAutocomplete>
</template>
</DropdownWrapper>

<div
v-if="expanded"
class="checkbox-list-wrapper"
>
<KCheckbox
v-for="option in filteredCategories"
:key="option.value"
data-test="option-checkbox"
:label="option.text"
:style="treeItemStyle(option)"
:checked="isSelected(option.value)"
@change="onChange(option.value)"
/>
<p
v-if="!filteredCategories.length"
:style="{ color: $themeTokens.annotation }"
>
{{ $tr('noCategoryFoundText') }}
</p>
</div>
</div>

</template>
Expand All @@ -113,12 +45,11 @@
import { getSortedCategories } from 'shared/utils/helpers';
import { createMultiSelectMessages } from 'shared/utils/multiSelectMessages';
import { communityChannelsStrings } from 'shared/strings/communityChannelsStrings';
import DropdownWrapper from 'shared/views/form/DropdownWrapper';
import { constantsTranslationMixin, metadataTranslationMixin } from 'shared/mixins';

export default {
name: 'CategoryOptions',
components: { KMultiSelect, KChip, DropdownWrapper },
components: { KMultiSelect, KChip },
mixins: [constantsTranslationMixin, metadataTranslationMixin],
props: {
/**
Expand All @@ -145,11 +76,6 @@
default: false,
},
},
data() {
return {
categoryText: null,
};
},
computed: {
categoriesList() {
const categories = getSortedCategories();
Expand All @@ -173,18 +99,6 @@
.map(([key]) => key);
return selectedValues;
},
nested() {
return !this.categoryText;
},
filteredCategories() {
const searchQuery = this.categoryText?.trim().toLowerCase();
if (!searchQuery) {
return this.categoriesList;
}
return this.categoriesList.filter(option =>
option.text.toLowerCase().includes(searchQuery),
);
},
messages() {
const { clearAllAction$ } = communityChannelsStrings;
return createMultiSelectMessages({
Expand All @@ -196,30 +110,6 @@
},
},
methods: {
treeItemStyle(item) {
const rule = this.$isRTL ? 'paddingRight' : 'paddingLeft';
return this.nested ? { [rule]: `${item.level * 24}px` } : {};
},
add(value) {
this.selected = {
...this.selected,
[value]: this.nodeIds,
};
},
remove(value) {
const newSelected = { ...this.selected };
Object.keys(this.selected)
.filter(selectedValue => selectedValue.startsWith(value))
.forEach(selectedValue => {
delete newSelected[selectedValue];
});
this.selected = newSelected;
},
removeAll() {
this.selected = {};
},
// Dropdown mode is only rendered when a single node is edited, so every
// selected category simply applies to all of nodeIds.
onKMultiSelectInput(newValues) {
Comment thread
rtibblesbot marked this conversation as resolved.
Resolved
this.selected = Object.fromEntries(newValues.map(value => [value, this.nodeIds]));
},
Expand All @@ -245,55 +135,6 @@
findDepth(val) {
return val.split('.').length - 1;
},
isSelected(value) {
// If the value is truthy (true or an array of nodeIds) then
// it is selected just if it is true (not an array)
if (this.selected[value] && this.selected[value].length === this.nodeIds.length) {
return true;
}

return this.isCheckboxSelectedByChildren(value);
},
/**
* Returns true if the given option should be selected thanks to its children.
* An option will be selected thanks to its children if:
* * One of the children is selected
* * It has several indeterminate children, but by joining all the contentNodes of the
* child options, together they constitute the same array of selected contentNodes.
*/
isCheckboxSelectedByChildren(optionId) {
const childrenOptions = Object.keys(this.selected)
.filter(selectedValue => selectedValue.startsWith(optionId))
.map(selectedValue => this.selected[selectedValue]);

if (childrenOptions.length === 0) {
return false; // No childen options
} else if (childrenOptions.length === 1) {
// just one child option, the value is deterrmined by if it is selected
return childrenOptions[0].length === this.nodeIds.length;
}

// Here multiple children are selected or indeterminate
if (childrenOptions.some(value => value.length === this.nodeIds.length)) {
// if some child value is selected for all nodes, then the parent option is selected
return true;
}

// Here all children options are mixed, we need to check if together
// the parent option is common for all nodes
const nodeIds = new Set();
childrenOptions.forEach(valueNodeIds => {
valueNodeIds.forEach(nodeId => nodeIds.add(nodeId));
});
return nodeIds.size === this.nodeIds.length;
},
onChange(optionId) {
if (this.isSelected(optionId)) {
this.remove(optionId);
} else {
this.add(optionId);
}
},
},
$trs: {
noCategoryFoundText: 'Category not found',
Expand All @@ -304,13 +145,3 @@
};

</script>


<style lang="scss" scoped>

.checkbox-list-wrapper {
height: 250px;
overflow-y: auto;
}

</style>
74 changes: 69 additions & 5 deletions ...contentcuration/frontend/shared/views/contentNodeFields/__tests__/categoryOptions.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,18 +135,27 @@ describe('CategoryOptions', () => {
});
});

describe('expanded mode', () => {
it('renders the flat checkbox list instead of KMultiSelect', () => {
describe('expanded mode (KMultiSelect)', () => {
it('shows the category list open, with no dropdown menu', async () => {
renderComponent({ expanded: true });

expect(screen.queryByRole('button', { name: openMenuAction$() })).not.toBeInTheDocument();
expect(screen.getByRole('checkbox', { name: DANCE_LABEL })).toBeInTheDocument();
expect(await screen.findByRole('option', { name: DANCE_LABEL })).toBeInTheDocument();
});

it('filters the category list as the user types', async () => {
renderComponent({ expanded: true });

await userEvent.type(screen.getByRole('combobox'), MUSIC_LABEL);

expect(screen.queryByRole('option', { name: DANCE_LABEL })).not.toBeInTheDocument();
expect(screen.getByRole('option', { name: MUSIC_LABEL })).toBeInTheDocument();
});

it('emits the added category applied to all edited nodes when checked', async () => {
const { emitted } = renderComponent({ expanded: true, nodeIds: [NODE_1] });

await userEvent.click(screen.getByRole('checkbox', { name: DANCE_LABEL }));
await userEvent.click(await screen.findByRole('option', { name: DANCE_LABEL }));

expect(lastInput(emitted)).toEqual({ [DANCE]: [NODE_1] });
});
Expand All @@ -158,7 +167,62 @@ describe('CategoryOptions', () => {
nodeIds: [NODE_1],
});

await userEvent.click(screen.getByRole('checkbox', { name: ARTS_LABEL }));
await userEvent.click(await screen.findByRole('option', { name: ARTS_LABEL }));

expect(lastInput(emitted)).toEqual({});
});

it('adds only the parent when a partially selected parent is clicked, same as dropdown mode', async () => {
const { emitted } = renderComponent({
expanded: true,
value: { [DANCE]: [NODE_1] },
nodeIds: [NODE_1],
});

await userEvent.click(
await screen.findByRole('option', { name: name => name.startsWith(SCHOOL_LABEL) }),
);

expect(lastInput(emitted)).toEqual({
[DANCE]: [NODE_1],
[SCHOOL]: [NODE_1],
});
});

it('does not carry over a category that is only partially applied across the edited nodes', async () => {
const { emitted } = renderComponent({
expanded: true,
value: { [MUSIC]: [NODE_1] },
nodeIds: [NODE_1, NODE_2],
});

await userEvent.click(await screen.findByRole('option', { name: DANCE_LABEL }));

expect(lastInput(emitted)).toEqual({ [DANCE]: [NODE_1, NODE_2] });
});

it('removes a category when its chip close button is clicked', async () => {
const { emitted } = renderComponent({
expanded: true,
value: { [DANCE]: [NODE_1] },
nodeIds: [NODE_1],
});

await userEvent.click(
screen.getByRole('button', { name: removeCategory$({ label: DANCE_PATH }) }),
);

expect(lastInput(emitted)).toEqual({});
});

it('emits an empty object when the selection is cleared', async () => {
Comment thread
rtibblesbot marked this conversation as resolved.
Resolved
const { emitted } = renderComponent({
expanded: true,
value: { [DANCE]: [NODE_1] },
nodeIds: [NODE_1],
});

await userEvent.click(screen.getByRole('button', { name: clearAllAction$() }));

expect(lastInput(emitted)).toEqual({});
});
Expand Down
Loading
Loading