Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,37 +1,37 @@
<template>

<div class="filter-pills">
<KButton
v-for="(entry, index) in entries"
:key="`${entry.type}-${index}`"
:data-testid="`${entry.type}-pill`"
:text="entry.label"
appearance="flat-button"
class="pill"
:appearanceOverrides="pillOverridesFor(entry)"
:disabled="loading"
@click="toggleFilter({ key: entry.termKey, value: entry.value })"
>
<template
v-if="entry.icon"
#icon
<fieldset class="filters-fieldset">
<legend class="visuallyhidden">{{ appliedFiltersGroupLabel$() }}</legend>
<label

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.

Should this be a list?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

do you mean, a

    instead that is styled the same way it currently is in the UI? honestly... that might be a good idea. i hadn't thought of it.

v-for="entry in entries"
:key="`${entry.termKey}:${entry.value}`"
:data-testid="`${entry.type}-pill`"
class="pill"
:class="$computedClass(pillPseudoStylesFor())"
:style="pillColorStyleFor(entry)"
>
<input
Comment thread
AlexVelezLl marked this conversation as resolved.
type="checkbox"
class="visuallyhidden"
:checked="isFilterActive(entry.termKey, entry.value)"
:aria-disabled="loading"
@click.prevent="handleToggle(entry)"
>
<KIcon
v-if="entry.icon"
:icon="entry.icon"
:color="entry.type === 'activity' ? null : $themeTokens.primary"
class="pill-icon"
/>
</template>
<template
v-if="iconAfterFor(entry)"
#iconAfter
>
<span class="link-text">{{ entry.label }}</span>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: .link-text styles nothing here — KDS scopes it as .link .link-text (buttons-and-links/buttons.scss:47) and this label has class pill. It carried over from KButton's internal markup.

The branch also newly owns this span, which makes dir="auto" possible for the first time: keyword pills carry the learner's own search text and language pills carry catalog lang_name values, neither of which goes through the translation layer.

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.

👀

<KIcon
v-if="iconAfterFor(entry)"
:icon="iconAfterFor(entry)"
class="pill-icon-after"
/>
</template>
</KButton>
</label>
</fieldset>
<span
v-if="hasAvailableLabels"
class="all-filters-group"
Expand All @@ -45,7 +45,7 @@
:text="allFilters$()"
appearance="flat-button"
class="pill"
:appearanceOverrides="pillOverridesFor({})"
:appearanceOverrides="pillColorStyleFor({})"
:disabled="loading"
@click="$emit('openFilters')"
>
Expand Down Expand Up @@ -80,7 +80,7 @@

<script>

import { computed } from 'vue';
import { computed, getCurrentInstance } from 'vue';
import { get } from '@vueuse/core';
import { themeTokens, themeBrand, themePalette } from 'kolibri-design-system/lib/styles/theme';
import { CategoriesLookup } from 'kolibri/constants';
Expand All @@ -93,6 +93,7 @@
export default {
name: 'HorizontalFilterPills',
setup() {
const instance = getCurrentInstance().proxy;
const {
availableLearningActivities,
availableLibraryCategories,
Expand Down Expand Up @@ -197,8 +198,16 @@
return isFilterActive(entry.termKey, entry.value) ? 'close' : null;
}

// Only theme-dependent styling lives here; layout is in the style block
function pillOverridesFor(entry) {
// Guarding here, rather than disabling the checkbox while loading, keeps
// it focusable so a keyboard toggle doesn't lose focus mid-search.
function handleToggle(entry) {
if (get(searchLoading)) {
return;
}
toggleFilter({ key: entry.termKey, value: entry.value });
}

function pillColorStyleFor(entry) {
if (isFilterActive(entry.termKey, entry.value)) {
return {
backgroundColor: themeBrand().primary.v_100,
Expand All @@ -216,19 +225,34 @@
};
}

const { allFilters$ } = searchAndFilterStrings;
// for a11y, the pill is a semantic checkbox and label
// but visually styled to match KButton for sighted users
function pillPseudoStylesFor() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: pillPseudoStylesFor() takes no arguments and returns the same object for every pill, so it's a computed, not a per-entry function — the For suffix invites a future caller to pass one. It's also invoked once per pill per render and $computedClass keys its cache on JSON.stringify(value), so the stringify runs each time. Only :focus-within needs to be dynamic; :hover, cursor and the loading opacity/pointer-events are static and belong in the scoped <style> block.

return {
':hover': get(searchLoading) ? {} : { backgroundColor: 'rgba(0,0,0,.1)' },
':focus-within': { ...instance.$coreOutline, outlineOffset: 0 },
...(get(searchLoading)
? { pointerEvents: 'none', cursor: 'default', opacity: 0.5 }
: { cursor: 'pointer' }),
};
}

const { allFilters$, appliedFiltersGroupLabel$ } = searchAndFilterStrings;
const { clearAllAction$ } = coreStrings;

return {
entries,
hasActiveFilters,
hasAvailableLabels,
isFilterActive,
iconAfterFor,
pillOverridesFor,
toggleFilter,
pillColorStyleFor,
pillPseudoStylesFor,
handleToggle,
clearSearch,
loading: searchLoading,
allFilters$,
appliedFiltersGroupLabel$,
clearAllAction$,
};
},
Expand All @@ -246,6 +270,13 @@
align-items: center;
}

// Strip the fieldset's default border/padding/min-width and let its pills
// lay out as direct flex children of .filter-pills, same as before grouping.
.filters-fieldset {
all: unset;
display: contents;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: display: contents (and :focus-within on line 224) sit below the repo's declared browser floor.

packages/browserslist-config-kolibri/index.js declares Chrome >= 49, ChromeAndroid >= 49, Samsung >= 5, iOS >= 10. display: contents needs Chrome 65 / Samsung 9.2 / iOS 11.1; :focus-within needs Chrome 60 / Samsung 8. Neither is polyfillable by postcss.

Verified the fallback in-page (all: unset alone computes to display: block), measured at 900px:

pill gaps .filter-pills height "All filters"
display: contents 8px 82px same row
fallback 0px 118px own row

The pill borders butt into one run-on strip. And with :focus-within unsupported, the focusable element is the 1px clipped input, so keyboard focus becomes invisible (WCAG 2.4.7) — KButton used :focus on the button itself, which has no floor.

Both alternatives avoid the property: put role="group" + aria-label on the existing .filter-pills flex container (it holds only filter controls), or keep the fieldset and give it the flex layout itself. Separately, display: contents on a grouping element is the case browsers historically dropped from the AX tree — it holds in Chrome (verified via CDP), but worth a Safari check, and getByRole('group') can't detect it since Testing Library never reads applied CSS.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: Unchanged: display: contents and :focus-within (224) below the browser floor; KButton's max-width: 100% no longer bounds the pill (281).

}

.filter-pills .pill {
display: inline-flex;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: Dropping KButton also dropped its .button base styles, and one of them was bounding the pill width.

.button (kolibri-design-system/lib/buttons-and-links/buttons.scss) supplied max-width: 100%; overflow: hidden; user-select: none and the colour transition. With white-space: nowrap kept but nothing bounding the width, a long keyword pill now pushes the whole document sideways. Measured at 412×915:

?keywords=how%20to%20prepare%20for%20the%20national%20mathematics%20exam
→ pill width 464px; documentElement.scrollWidth 488 vs clientWidth 412

The trailing close icon scrolls off with it, so the filter can only be removed via "Clear all". On develop the pill was clipped to the container. Re-adding max-width: 100%; overflow: hidden restores the bound.

Two smaller losses in the same row, visible next to the unchanged "All filters" KButton: pill text is now selectable (user-select: auto), and the rgba(0,0,0,.1) hover snaps in while its neighbour still fades. The label also renders 36px against "All filters"'s 38px — KButton wraps its icon in a 20px span.icon-container and the label renders the 18px <svg> directly, so min-height: 0 lets it shrink; min-height: 20px here brings them back level (verified in-page).

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.

Yeah, for long keywords, we don't handle overflow properly.
Image

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is an edge case I missed -- thanks Alex for the screenshot, I will update this

align-items: center;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

<form
class="library-search-bar"
role="search"
@submit.prevent="handleSubmit"
>
<div
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ref } from 'vue';
import { render, screen, fireEvent } from '@testing-library/vue';
import { render, screen, fireEvent, within } from '@testing-library/vue';
import { Categories } from 'kolibri/constants';
import { coreStrings } from 'kolibri/uiText/commonCoreStrings';
import { searchAndFilterStrings } from 'kolibri-common/strings/searchAndFilterStrings';
Expand All @@ -15,7 +15,7 @@ const {
mathematics$,
clearAllAction$,
} = coreStrings;
const { allFilters$ } = searchAndFilterStrings;
const { allFilters$, appliedFiltersGroupLabel$ } = searchAndFilterStrings;

const KEYWORD_FILTER = 'hummingbirds';

Expand Down Expand Up @@ -67,35 +67,113 @@ function renderComponent(provides = {}, props = {}) {
}

describe('HorizontalFilterPills', () => {
describe('grouping', () => {
it('groups the filter checkboxes under a named group', () => {
renderComponent();
const group = screen.getByRole('group', { name: appliedFiltersGroupLabel$() });
expect(within(group).getByRole('checkbox', { name: create$() })).toBeInTheDocument();
expect(within(group).getByRole('checkbox', { name: school$() })).toBeInTheDocument();
});

it('does not include the all-filters or clear-all actions in the group', () => {
renderComponent({
appliedFilters: () => [{ key: 'learning_activities', value: 'UXADWcXZ' }],
});
const group = screen.getByRole('group', { name: appliedFiltersGroupLabel$() });
expect(within(group).queryByRole('button', { name: allFilters$() })).not.toBeInTheDocument();
expect(
within(group).queryByRole('button', { name: clearAllAction$() }),
).not.toBeInTheDocument();
});
});

describe('activity pills', () => {
it('renders a pill for each available activity', () => {
it('renders a checkbox for each available activity', () => {
renderComponent();
expect(screen.getAllByTestId('activity-pill')).toHaveLength(3);
expect(screen.getByRole('button', { name: create$() })).toBeInTheDocument();
expect(screen.getByRole('button', { name: explore$() })).toBeInTheDocument();
expect(screen.getByRole('button', { name: listen$() })).toBeInTheDocument();
expect(screen.getByRole('checkbox', { name: create$() })).toBeInTheDocument();
expect(screen.getByRole('checkbox', { name: explore$() })).toBeInTheDocument();
expect(screen.getByRole('checkbox', { name: listen$() })).toBeInTheDocument();
});

it('calls toggleFilter when an activity pill is clicked', async () => {
const { toggleFilter } = renderComponent();
await fireEvent.click(screen.getByRole('button', { name: create$() }));
await fireEvent.click(screen.getByRole('checkbox', { name: create$() }));
expect(toggleFilter).toHaveBeenCalledWith(
expect.objectContaining({ key: 'learning_activities', value: 'UXADWcXZ' }),
);
});

it('reflects the applied state as checked', () => {
renderComponent({
appliedFilters: () => [{ key: 'learning_activities', value: 'UXADWcXZ' }],
isFilterActive: (key, value) => key === 'learning_activities' && value === 'UXADWcXZ',
});
expect(screen.getByRole('checkbox', { name: create$() })).toBeChecked();
expect(screen.getByRole('checkbox', { name: explore$() })).not.toBeChecked();
});

it('keeps checked in sync after unapplying the first of two applied activities', async () => {
const applied = ref([]);
renderComponent({
appliedFilters: () => applied.value,
isFilterActive: (key, value) => applied.value.some(f => f.key === key && f.value === value),
toggleFilter: ({ key, value }) => {
const idx = applied.value.findIndex(f => f.key === key && f.value === value);
applied.value =
idx === -1
? [...applied.value, { key, value }]
: applied.value.filter((_, i) => i !== idx);
},
});

await fireEvent.click(screen.getByRole('checkbox', { name: explore$() }));
await fireEvent.click(screen.getByRole('checkbox', { name: listen$() }));
await fireEvent.click(screen.getByRole('checkbox', { name: explore$() }));

expect(screen.getByRole('checkbox', { name: listen$() })).toBeChecked();
expect(screen.getByRole('checkbox', { name: explore$() })).not.toBeChecked();
});
});

describe('while loading', () => {
it('keeps the checkbox enabled so a keyboard toggle does not lose focus', () => {
renderComponent({ searchLoading: ref(true) });
expect(screen.getByRole('checkbox', { name: create$() })).toBeEnabled();
});

it('ignores a toggle started while a previous one is still loading', async () => {
const { toggleFilter } = renderComponent({ searchLoading: ref(true) });
await fireEvent.click(screen.getByRole('checkbox', { name: create$() }));
expect(toggleFilter).not.toHaveBeenCalled();
});

it('does not let an ignored click flip the checkbox out of sync with the applied filters', async () => {
renderComponent({ searchLoading: ref(true) });
await fireEvent.click(screen.getByRole('checkbox', { name: create$() }));
expect(screen.getByRole('checkbox', { name: create$() })).not.toBeChecked();
});

it('marks the checkbox aria-disabled for assistive tech', () => {
renderComponent({ searchLoading: ref(true) });
expect(screen.getByRole('checkbox', { name: create$() })).toHaveAttribute(
'aria-disabled',
'true',
);
});
});

describe('category pills', () => {
it('renders a pill for each available category', () => {
it('renders a checkbox for each available category', () => {
renderComponent();
expect(screen.getAllByTestId('category-pill')).toHaveLength(2);
expect(screen.getByRole('button', { name: school$() })).toBeInTheDocument();
expect(screen.getByRole('button', { name: dailyLife$() })).toBeInTheDocument();
expect(screen.getByRole('checkbox', { name: school$() })).toBeInTheDocument();
expect(screen.getByRole('checkbox', { name: dailyLife$() })).toBeInTheDocument();
});

it('calls toggleFilter when a category pill is clicked', async () => {
const { toggleFilter } = renderComponent();
await fireEvent.click(screen.getByRole('button', { name: school$() }));
await fireEvent.click(screen.getByRole('checkbox', { name: school$() }));
expect(toggleFilter).toHaveBeenCalledWith(
expect.objectContaining({ key: 'categories', value: Categories.SCHOOL }),
);
Expand All @@ -112,6 +190,7 @@ describe('HorizontalFilterPills', () => {
});
const pill = screen.getByTestId('category-pill');
expect(pill).toHaveTextContent(mathematics$());
expect(screen.getByRole('checkbox', { name: mathematics$() })).toBeChecked();
expect(screen.queryByText(SUBCATEGORY_VALUE)).not.toBeInTheDocument();
});
});
Expand Down Expand Up @@ -139,7 +218,7 @@ describe('HorizontalFilterPills', () => {
key === 'learning_activities' ? value === '#j8L0eq3' : false,
});
expect(screen.getAllByTestId('activity-pill')).toHaveLength(1);
expect(screen.getByRole('button', { name: explore$() })).toBeInTheDocument();
expect(screen.getByRole('checkbox', { name: explore$() })).toBeInTheDocument();
expect(screen.queryAllByTestId('category-pill')).toHaveLength(0);
});

Expand All @@ -150,7 +229,7 @@ describe('HorizontalFilterPills', () => {
isFilterActive: (key, value) => key === 'learning_activities' && value === 'UXADWcXZ',
});
expect(screen.getAllByTestId('activity-pill')).toHaveLength(1);
expect(screen.getByRole('button', { name: create$() })).toBeInTheDocument();
expect(screen.getByRole('checkbox', { name: create$() })).toBeInTheDocument();
});
});

Expand All @@ -161,7 +240,7 @@ describe('HorizontalFilterPills', () => {
isFilterActive: (key, value) => key === 'keywords' && value === KEYWORD_FILTER,
});
expect(screen.getByTestId('keyword-pill')).toBeInTheDocument();
expect(screen.getByRole('button', { name: KEYWORD_FILTER })).toBeInTheDocument();
expect(screen.getByRole('checkbox', { name: KEYWORD_FILTER })).toBeChecked();
});

it('renders an active filter from a non-catalog dimension', () => {
Expand All @@ -170,7 +249,7 @@ describe('HorizontalFilterPills', () => {
isFilterActive: (key, value) => key === 'grade_levels' && value === 'basic_skills',
});
expect(screen.getByTestId('grade_levels-pill')).toBeInTheDocument();
expect(screen.getByRole('button', { name: basicSkills$() })).toBeInTheDocument();
expect(screen.getByRole('checkbox', { name: basicSkills$() })).toBeChecked();
});

it('labels an applied language with its name, not the raw code', () => {
Expand All @@ -183,7 +262,7 @@ describe('HorizontalFilterPills', () => {
isFilterActive: (key, value) => key === 'languages' && value === language.id,
});
expect(screen.getByTestId('language-pill')).toBeInTheDocument();
expect(screen.getByRole('button', { name: language.lang_name })).toBeInTheDocument();
expect(screen.getByRole('checkbox', { name: language.lang_name })).toBeChecked();
expect(screen.queryByText(language.id)).not.toBeInTheDocument();
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ describe('LibrarySearchBar', () => {
});

describe('rendering', () => {
it('exposes a search landmark so it can be reached independent of the skip link', () => {
renderComponent();
expect(screen.getByRole('search')).toBeInTheDocument();
});

it('renders a search input', () => {
renderComponent();
expect(screen.getByRole('combobox', { name: searchLabel$() })).toBeInTheDocument();
Expand Down
10 changes: 9 additions & 1 deletion kolibri/plugins/learn/frontend/views/LibraryPage/index.vue
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,15 @@
:deviceId="deviceId"
:route="back"
>
<main class="main-grid">
<main
class="main-grid"
data-skip-nav-target
:aria-label="displayingSearchResults ? null : channelsLabel"
>
<!--
Overrides SkipNavigationLink's default of focusing the first h1: the
search bar precedes the h1 here, so focusing the h1 would skip over it.
-->
<!-- Search header: search bar + filter pills grouped together -->
<div
class="search-header"
Expand Down
Loading
Loading