feat(cdk-experimental/ui-patterns): listbox ui pattern

[wagnermaciel]

No description provided.

[jelbourn]

First batch of comments, I'm still going

[jelbourn]

I'm not sure that pointer events should always preventDefault and stopPropogation the way that keyboard events do. What's your thinking here?

[mmalerba]

There was no thinking on this, I wasn't really sure what the default behaviors should be. This is just what I put in the prototype.

Thinking about it more right now, I would suggest:
keyboard:

• preventDefualt (yes)
• stopPropagation (yes? - motivation for this is things like stacks of dialog, menu, etc where escape should probably close the top one, not all of them)

mouse:

• preventDefault (no - e.g. this is probably not desired for mousedown where preventDefault would cancel focusing of the element)
• stopPropagation (maybe? not really sure about this one)

Anyways that's just my thought for the default defaults. People could always create a manager with different defaults new KeydownEventManager({preventDefault: false}) or register a handler with a different setting than the manager's default manager.on('x', () => {...}, {preventDefault: false})

[crisbeto]

Why don't we call this patterns? The ui- seems a bit redundant.

[wagnermaciel]

@jelbourn Do you recall the reason for keeping the ui- prefix?

[mmalerba]

I like the ui-, patterns by itself makes me think of like GoF design patterns or something

[jelbourn]

Yeah, the thinking was that just "patterns" by itself was too generic

[crisbeto]

Isn't everything we do related to building UI though? It seems a bit redundant to have it in the name. Then again, if we don't intend to make this public it might not matter 🤷‍♂️

[crisbeto]

Should we make the label required so we don't have to hit the DOM every time the computed runs?

[wagnermaciel]

IMO the devex is worth the performance cost in this case. If I understand correctly, this computed will only rerun if the label is set/unset or if the element changes (which I would assume is rare)

[crisbeto]

I think that initially it runs so the runtime can figure out the dependencies and afterwards it only runs on changes. That being said, it might be a bit misleading because it's not going to update if the textContent changes. I expect it might come up, given that we had to account for it in mat-option: https://github.com/angular/components/blob/main/src/material/core/option/option.ts#L267

My thinking was that it's easier to introduce it later than having consumers depend on it and us having to change it later.

[wagnermaciel]

That's a good point, I hadn't considered the text content changing. I still lean against making the label required because I think it's too much boilerplate. Would it be ok to leave this for a follow-up PR so that we can discuss when we review the public API design doc?

[crisbeto]

Another option can be to only derive the label from the input and if there isn't one, we just don't do typeahead.

[devversion]

LGTM for dev-infra; leaving naming to others

[jelbourn]

Not necessarily for this PR, but one thing we should discuss in a bit more detail is how we're managing selected IDs, since I worry that the array isn't the best data structure vs. a Set, but we don't have a pattern established yet for a Set that plays well with signals

[jelbourn]

LGTM aside from a last few minor comments

[jelbourn]

This can be a TODO / cleanup for a next PR, but these generally shouldn't be computed for things that won't change.

I think the reason that you're doing this is because the e.g. OptionInputs expects a signal. However, you don't really want those to be a Signal<T>, but instead a () => T (which would also make the types work for Wiz) . You could optionally introduce a convenience type for this that's something like

export type Reactive<T> = () => T;

We probably want to do this before build too many more ___Input types