Skip to content

Commit

Permalink
refactor(core): don't bind this in controller options
Browse files Browse the repository at this point in the history
  • Loading branch information
@bennypowers
bennypowers committed Aug 15, 2024
1 parent cc177ff commit 46442ed
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 74 deletions.
6 changes: 3 additions & 3 deletions core/pfe-core/controllers/activedescendant-controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ export interface ActivedescendantControllerOptions<
* If you provide this callback, ActivedescendantController will call it on your item with the
* active state. You may use this to set active styles.
*/
setItemActive?(this: Item, active: boolean): void;
setItemActive?(item: Item, active: boolean): void;
/**
* Optional callback to retrieve the value from an option element.
* By default, retrieves the `value` attribute, or the text content.
* @see https://developer.mozilla.org/en-US/docs/Web/API/HTMLOptionElement
*/
getItemValue?(this: Item): string;
getItemValue?(item: Item): string;
}

/**
Expand Down Expand Up @@ -134,7 +134,7 @@ export class ActivedescendantController<
super.atFocusedItemIndex = index;
const item = this._items.at(this.atFocusedItemIndex);
for (const _item of this.items) {
this.options.setItemActive?.call(_item, _item === item);
this.options.setItemActive?.(_item, _item === item);
}
const container = this.options.getActiveDescendantContainer();
if (!ActivedescendantController.supportsCrossRootActiveDescendant) {
Expand Down
64 changes: 32 additions & 32 deletions core/pfe-core/controllers/combobox-controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,6 @@ type AllOptions<Item extends HTMLElement> =

type Lang = typeof ComboboxController['langs'][number];

function getItemValue<Item extends HTMLElement>(this: Item): string {
if ('value' in this && typeof this.value === 'string') {
return this.value;
} else {
return '';
}
}

function deepClosest(element: Element | null, selector: string) {
let closest = element?.closest(selector);
let root = element?.getRootNode();
Expand All @@ -45,31 +37,39 @@ function deepClosest(element: Element | null, selector: string) {
return closest;
}

function isItemFiltered<Item extends HTMLElement>(this: Item, value: string): boolean {
return !getItemValue.call(this)
function getItemValue<Item extends HTMLElement>(item: Item): string {
if ('value' in item && typeof item.value === 'string') {
return item.value;
} else {
return '';
}
}

function isItemFiltered<Item extends HTMLElement>(item: Item, value: string): boolean {
return !getItemValue(item)
.toLowerCase()
.startsWith(value.toLowerCase());
}

function setItemHidden(this: HTMLElement, hidden: boolean) {
this.hidden = hidden;
function setItemHidden(item: HTMLElement, hidden: boolean) {
item.hidden = hidden;
}

function setComboboxValue(this: HTMLElement, value: string): void {
if (!('value' in this)) {
function setComboboxValue(item: HTMLElement, value: string): void {
if (!('value' in item)) {
// eslint-disable-next-line no-console
return console.warn(`Cannot set value on combobox element ${this.localName}`);
return console.warn(`Cannot set value on combobox element ${item.localName}`);
} else {
this.value = value;
item.value = value;
}
}

function getComboboxValue(this: HTMLElement): string {
if ('value' in this && typeof this.value === 'string') {
return this.value;
function getComboboxValue(combobox: HTMLElement): string {
if ('value' in combobox && typeof combobox.value === 'string') {
return combobox.value;
} else {
// eslint-disable-next-line no-console
return console.warn(`Cannot get value from combobox element ${this.localName}`), '';
return console.warn(`Cannot get value from combobox element ${combobox.localName}`), '';
}
}

Expand Down Expand Up @@ -118,28 +118,28 @@ export interface ComboboxControllerOptions<Item extends HTMLElement> extends
* of the item, as if it implemented the `<option>` element's interface.
* @see https://developer.mozilla.org/en-US/docs/Web/API/HTMLOptionElement
*/
getItemValue?(this: Item): string;
getItemValue?(item: Item): string;
/**
* Optional callback, called on the combobox input element to set its value.
* by default, returns the element's `value` DOM property.
*/
getComboboxValue?(this: HTMLElement): string;
getComboboxValue?(combobox: HTMLElement): string;
/**
* Optional callback, called on the combobox input element to set its value.
* by default, sets the element's `value` DOM property.
*/
setComboboxValue?(this: HTMLElement, value: string): void;
setComboboxValue?(item: HTMLElement, value: string): void;
/**
* Called on each item, with the combobox input, to determine if the item should be shown in the
* listbox or filtered out. Return false to hide the item. By default, checks whether the item's
* value starts with the input value (when both are lowercased).
*/
isItemFiltered?(this: Item, value: string): boolean;
isItemFiltered?(item: Item, value: string): boolean;
/**
* Called on each item when the filter changes.
* By default, toggles the `hidden` attribute on the item
*/
setItemHidden?(this: Item, hidden: boolean): void;
setItemHidden?(item: Item, hidden: boolean): void;
}

/**
Expand Down Expand Up @@ -499,14 +499,14 @@ export class ComboboxController<

// TODO(bennypowers): perhaps move this to ActivedescendantController
#announce(item: Item) {
const value = this.options.getItemValue.call(item);
const value = this.options.getItemValue(item);
ComboboxController.#alert?.remove();
const fragment = ComboboxController.#alertTemplate.content.cloneNode(true) as DocumentFragment;
ComboboxController.#alert = fragment.firstElementChild as HTMLElement;
let text = value;
const lang = deepClosest(this.#listbox, '[lang]')?.getAttribute('lang') ?? 'en';
const langKey = lang?.match(ComboboxController.langsRE)?.at(0) as Lang ?? 'en';
if (this.options.isItemDisabled.call(item)) {
if (this.options.isItemDisabled(item)) {
text += ` (${this.#translate('dimmed', langKey)})`;
}
if (this.#lb.isSelected(item)) {
Expand All @@ -530,10 +530,10 @@ export class ComboboxController<
for (const item of this.items) {
const hidden =
!!this.options.isExpanded()
&& !!(value = this.options.getComboboxValue.call(this.#input))
&& this.options.isItemFiltered?.call(item, value)
&& !!(value = this.options.getComboboxValue(this.#input))
&& this.options.isItemFiltered?.(item, value)
|| false;
this.options.setItemHidden.call(item, hidden);
this.options.setItemHidden(item, hidden);
}
}
}
Expand Down Expand Up @@ -606,7 +606,7 @@ export class ComboboxController<
break;
case 'Escape':
if (!this.options.isExpanded()) {
this.options.setComboboxValue.call(this.#input, '');
this.options.setComboboxValue(this.#input, '');
this.host.requestUpdate();
}
this.#hide();
Expand Down Expand Up @@ -688,7 +688,7 @@ export class ComboboxController<
if (eventItem
&& !this.multi
&& this.options.isExpanded()
&& !this.options.isItemDisabled.call(eventItem)
&& !this.options.isItemDisabled(eventItem)
) {
this.#hide();
this.#button?.focus();
Expand Down
36 changes: 18 additions & 18 deletions core/pfe-core/controllers/listbox-controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export interface ListboxControllerOptions<Item extends HTMLElement> {
* element with the selected state.
* Callers **must** ensure that the correct ARIA state is set.
*/
setItemSelected?(this: Item, selected: boolean): void;
setItemSelected?(item: Item, selected: boolean): void;
/**
* Optional predicate to ascertain whether a custom element item is disabled or not
* By default, if the item matches any of these conditions, it is considered disabled:
Expand All @@ -27,7 +27,7 @@ export interface ListboxControllerOptions<Item extends HTMLElement> {
* 2. it has the `disabled` attribute present
* 3. it matches the `:disabled` pseudo selector
*/
isItemDisabled?(this: Item): boolean;
isItemDisabled?(item: Item): boolean;
/**
* Predicate which determines if a given element is in fact an item
* instead of e.g a presentational divider. By default, elements must meet the following criteria
Expand Down Expand Up @@ -63,13 +63,14 @@ export interface ListboxControllerOptions<Item extends HTMLElement> {

/**
* This is the default method for setting the selected state on an item element
* @param item the item
* @param selected is this item selected
*/
function setItemSelected<Item extends HTMLElement>(this: Item, selected: boolean) {
function setItemSelected<Item extends HTMLElement>(item: Item, selected: boolean) {
if (selected) {
this.setAttribute('aria-selected', 'true');
item.setAttribute('aria-selected', 'true');
} else {
this.removeAttribute('aria-selected');
item.removeAttribute('aria-selected');
}
}

Expand All @@ -92,12 +93,12 @@ export function isItem<Item extends HTMLElement>(item: EventTarget | null): item
* @param item possibly disabled item
* @package do not import this outside of `@patternfly/pfe-core`, it is subject to change at any time
*/
export function isItemDisabled<Item extends HTMLElement>(this: Item): boolean {
return ('disabled' in this && typeof this.disabled === 'boolean' && this.disabled)
|| this.getAttribute('aria-disabled') === 'true'
|| this.hasAttribute('disabled')
|| this.hasAttribute('inert')
|| this.matches(':disabled');
export function isItemDisabled<Item extends HTMLElement>(item: Item): boolean {
return ('disabled' in item && typeof item.disabled === 'boolean' && item.disabled)
|| item.getAttribute('aria-disabled') === 'true'
|| item.hasAttribute('disabled')
|| item.hasAttribute('inert')
|| item.matches(':disabled');
}

let constructingAllowed = false;
Expand Down Expand Up @@ -211,7 +212,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
if (!arraysAreEquivalent(selected, Array.from(this.#selectedItems))) {
this.#selectedItems = new Set(selected);
for (const item of this.items) {
this.#options.setItemSelected.call(item, this.#selectedItems.has(item));
this.#options.setItemSelected(item, this.#selectedItems.has(item));
}
this.host.requestUpdate();
}
Expand Down Expand Up @@ -417,7 +418,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
#onClick = (event: MouseEvent) => {
const item = this.#getItemFromEvent(event);
this.#shiftStartingItem ??= this.#getItemFromEvent(event);
if (item && !this.#options.isItemDisabled.call(item)) {
if (item && !this.#options.isItemDisabled(item)) {
// Case: single select?
// just reset the selected list.
if (!this.multi) {
Expand Down Expand Up @@ -489,8 +490,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
if (event.ctrlKey
&& (event.target === this.container
|| this.#options.isItem(event.target))) {
const selectableItems = this.items.filter(item =>
!this.#options.isItemDisabled.call(item));
const selectableItems = this.items.filter(item => !this.#options.isItemDisabled(item));
if (arraysAreEquivalent(this.selected, selectableItems)) {
this.selected = [];
} else {
Expand All @@ -514,7 +514,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
this.selected = this.items.filter((x, i) =>
this.#selectedItems.has(x)
|| i === this.items.indexOf(item) - 1)
.filter(x => !this.#options.isItemDisabled.call(x));
.filter(x => !this.#options.isItemDisabled(x));
}
break;
case 'ArrowDown':
Expand All @@ -523,7 +523,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
this.selected = this.items.filter((x, i) =>
this.#selectedItems.has(x)
|| i === this.items.indexOf(item) + 1)
.filter(x => !this.#options.isItemDisabled.call(x));
.filter(x => !this.#options.isItemDisabled(x));
}
break;
case ' ':
Expand All @@ -544,7 +544,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
};

#selectItem(item: Item, shiftDown = false) {
if (this.#options.isItemDisabled.call(item)) {
if (this.#options.isItemDisabled(item)) {
return;
} else if (this.multi && shiftDown) {
// update starting item for other multiselect
Expand Down
8 changes: 2 additions & 6 deletions core/pfe-core/controllers/test/combobox-controller.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,8 @@ abstract class TestCombobox extends ReactiveElement {
isExpanded: () => !this.listbox.hidden,
requestShowListbox: () => void (this.listbox.hidden = false),
requestHideListbox: () => void (this.listbox.hidden = true),
setItemActive(active) {
this.classList.toggle('active', active);
},
setItemSelected(selected) {
this.selected = selected;
},
setItemActive: (item, active) => item.classList.toggle('active', active),
setItemSelected: (item, selected) => item.selected = selected,
});

@query('#listbox') listbox!: HTMLElement;
Expand Down
20 changes: 5 additions & 15 deletions elements/pf-select/pf-select.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,22 +127,12 @@ export class PfSelect extends LitElement {
getToggleButton: () => this._toggleButton ?? null,
getComboboxInput: () => this._toggleInput ?? null,
isExpanded: () => this.expanded,
requestShowListbox: () =>
this.expanded ||= true,
requestHideListbox: () =>
((this.expanded &&= false), true),
setItemHidden(hidden) {
if (this.id !== 'placeholder') {
this.hidden = hidden;
}
},
requestShowListbox: () => void (this.expanded ||= true),
requestHideListbox: () => void (this.expanded &&= false),
setItemHidden: (item, hidden) => (item.id !== 'placeholder') && void (item.hidden = hidden),
isItem: item => item instanceof PfOption,
setItemActive(active) {
this.active = active;
},
setItemSelected(selected) {
this.selected = selected;
},
setItemActive: (item, active) => item.active = active,
setItemSelected: (item, selected) => item.selected = selected,
});

/**
Expand Down

0 comments on commit 46442ed

Please sign in to comment.