support listbox with multiple selection #171
Conversation
| get selectedOptionGuid() { | ||
| return this.optionElements[this.selectedOptionIndex]?.id; | ||
| get selectedOptionGuids() { | ||
| return Array.from(this.selectedOptionIndexes).map( |
There was a problem hiding this comment.
way make an array that ends up being a separate array each access?
does this need to be a getter?
can we access the optionElements directly?
| @@ -181,15 +187,27 @@ export default class ListboxComponent extends Component { | |||
| optionElement.setAttribute('data-index', this.optionElements.length - 1); | |||
There was a problem hiding this comment.
any chance you'd be willing to do a PR that removes all this "registerElement" stuff? it's very side-effect heavy and causes double (or more!) renders
…ns. fix unselecting option on multi-select.
There was a problem hiding this comment.
Changes look good / targeted to me -- at least, inline with the existing conventions of this component.
If you have time though, it'd be well worth while to modernize / reduce re-renders of the components / back-and-forth reference passing -- but that's totally out of scope for this PR. <3
Thanks!
|
|
||
| this.scrollIntoView(optionElement); | ||
| } | ||
| } | ||
|
|
||
| if (!this.selectedOptionIndex) { | ||
| scheduleOnce('afterRender', this, this.setDefaultActiveOption); |
There was a problem hiding this comment.
can we avoid hacking runloop usage?
runloop is kinda anti-patterny and causes performance problems
|
A couple more changes that this PR makes:
|
|
Hey @miguelcobain 👋 |
|
Is there anything that needs to be done on this still or is it something that can be merged? I'm in need of multi-select support for the listbox and was going to fork this repo to get it. But I would love for this to actually be merged and released. |
|
I would really love for this to get merged as I have a fork of this addon just for this PR. Is there any reason this can't be merged? Does it need updates before it can? |
|
@NullVoxPopuli Is this something that can be merged? |
|
There are conflicts, I'd say it's up to @GavinJoyce and @miguelcobain Personally, my efforts are focused on not mimicking tailwind's patterns over here: |
I ported all the tests for the multi-select from https://github.com/tailwindlabs/headlessui/blob/main/packages/%40headlessui-react/src/components/listbox/listbox.test.tsx
Let me know your feedback, please.