-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat(cdk-experimental/combobox): introduce new signals-based combobox #31872
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: main
Are you sure you want to change the base?
Conversation
Deployed dev-app for ea2d807 to: https://ng-dev-previews-comp--pr-angular-components-31872-dev-jwmz3vug.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
904893f
to
d9ade63
Compare
9df4134
to
3eac760
Compare
|
||
return this._panelPortal; | ||
constructor() { | ||
(this.combobox.pattern.inputs.inputEl as WritableSignal<HTMLInputElement>).set( |
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.
Maybe adding an inputEl
property to CdkCombobox and assign that value instead of deep assignment?
} | ||
|
||
/** Handles input events for the combobox. */ | ||
onInput(event: Event) { |
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.
I kinda feel the filtering should be a business logic not handled by Angular Aria. The combobox with filtering can be achieved like
<div cdkCombobox>
<input cdkCombobox [(ngModel)]="inputValue" />
<div popover>
<ng-template cdkComboboxPopupContainer>
<ul cdkListbox [(value)]="selection">
@for (item of items; track item) {
if (match(inputValue, item)) {
<li cdkOption>{{ item }}</li>
}
}
</ul>
</ng-template>
</div>
</div>
so that Combobox only takes care of what's been rendered in the popup, and we also don't need to use inert
to implicitly tell what's being filtered out.
const activeItem = untracked(() => inputs.activeItem()); | ||
|
||
if (!items.some(i => i === activeItem) && activeItem) { | ||
this.pattern.listBehavior.unfocus(); |
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.
I am trying to understand the logic here (and the same logic in the tree). The logic seems to unset an active item if the active item is no longer in the list, but in what situation this applies? Removing it doesn't seem to affect the local build to me.
|
||
if (items && value.some(v => !items.some(i => i.value() === v))) { | ||
this.value.set(value.filter(v => items.some(i => i.value() === v))); | ||
this._popup?.combobox?.pattern?.inputs.value.set(this.value()[0]); |
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.
Probably should pass selected value back to combobox via popup controls instead of implementing them in both listbox and tree.
isItemCollapsible = () => this.activeItem()?.parent() instanceof TreeItemPattern; | ||
|
||
/** The ARIA role for the tree. */ | ||
role = () => 'tree' as const; |
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.
nit: maybe worth adding this to the TreePattern
and also use this property for host binding? [attr.role]="pattern.role()"
role = () => 'tree' as const; | ||
|
||
/* The id of the active (focused) item in the tree. */ | ||
activeId = computed(() => this.listBehavior.activedescendant()); |
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.
Originally I wanted to make the Controls/Actions
interface to only contain callback functions so 3p developers can embed non-Angular components (in different component life-cycle), and all the states/props are done via template inputs. Now it seems like we would need developers to implement states using Signals.
I am fine with the decision, but just want to mention what was the intention of the Controls/Actions interface.
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.
Ah, that's a great point. We should discuss whether the Controls
should avoid using signals. A simple workaround would be to change this to just getActiveId
so it is communicated as more of a "read" than a "binding"
*/ | ||
export class TreeItemPattern<V> implements ExpansionItem { | ||
/** Whether the option is inert. */ | ||
inert = signal<true | null>(null); |
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.
It's fairly difficult to track where changes the inert
value. I found combobox-tree and combobox-listbox control this value, but unsure how it's working within tree alone. Should we add a comment of how this attribute is used and how it's different from expansion visibility.
Uh oh!
There was an error while loading. Please reload this page.