-
-
Notifications
You must be signed in to change notification settings - Fork 968
Learn searchbar a11y updates #15267
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: develop
Are you sure you want to change the base?
Learn searchbar a11y updates #15267
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,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 | ||||||||||||||
| v-for="entry in entries" | ||||||||||||||
| :key="`${entry.termKey}:${entry.value}`" | ||||||||||||||
| :data-testid="`${entry.type}-pill`" | ||||||||||||||
| class="pill" | ||||||||||||||
| :class="$computedClass(pillPseudoStylesFor())" | ||||||||||||||
| :style="pillColorStyleFor(entry)" | ||||||||||||||
| > | ||||||||||||||
| <input | ||||||||||||||
|
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> | ||||||||||||||
|
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: The branch also newly owns this span, which makes
Member
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. 👀 |
||||||||||||||
| <KIcon | ||||||||||||||
| v-if="iconAfterFor(entry)" | ||||||||||||||
| :icon="iconAfterFor(entry)" | ||||||||||||||
| class="pill-icon-after" | ||||||||||||||
| /> | ||||||||||||||
| </template> | ||||||||||||||
| </KButton> | ||||||||||||||
| </label> | ||||||||||||||
| </fieldset> | ||||||||||||||
| <span | ||||||||||||||
| v-if="hasAvailableLabels" | ||||||||||||||
| class="all-filters-group" | ||||||||||||||
|
|
@@ -45,7 +45,7 @@ | |||||||||||||
| :text="allFilters$()" | ||||||||||||||
| appearance="flat-button" | ||||||||||||||
| class="pill" | ||||||||||||||
| :appearanceOverrides="pillOverridesFor({})" | ||||||||||||||
| :appearanceOverrides="pillColorStyleFor({})" | ||||||||||||||
| :disabled="loading" | ||||||||||||||
| @click="$emit('openFilters')" | ||||||||||||||
| > | ||||||||||||||
|
|
@@ -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'; | ||||||||||||||
|
|
@@ -93,6 +93,7 @@ | |||||||||||||
| export default { | ||||||||||||||
| name: 'HorizontalFilterPills', | ||||||||||||||
| setup() { | ||||||||||||||
| const instance = getCurrentInstance().proxy; | ||||||||||||||
| const { | ||||||||||||||
| availableLearningActivities, | ||||||||||||||
| availableLibraryCategories, | ||||||||||||||
|
|
@@ -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, | ||||||||||||||
|
|
@@ -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() { | ||||||||||||||
|
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: |
||||||||||||||
| 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$, | ||||||||||||||
| }; | ||||||||||||||
| }, | ||||||||||||||
|
|
@@ -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; | ||||||||||||||
|
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:
Verified the fallback in-page (
The pill borders butt into one run-on strip. And with Both alternatives avoid the property: put
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: Unchanged: |
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| .filter-pills .pill { | ||||||||||||||
| display: inline-flex; | ||||||||||||||
|
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: Dropping
The trailing Two smaller losses in the same row, visible next to the unchanged "All filters"
Member
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.
Member
Author
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. That is an edge case I missed -- thanks Alex for the screenshot, I will update this |
||||||||||||||
| align-items: center; | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| <form | ||
| class="library-search-bar" | ||
| role="search" | ||
| @submit.prevent="handleSubmit" | ||
| > | ||
| <div | ||
|
|
||

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.
Should this be a list?
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.
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.