✨ Implement `<Listbox />` component #76

dmcnamara-eng · 2021-08-31T14:21:42Z

✨ What Changed and Why

This PR is a functionally complete implementation of a <Listbox /> component based on https://headlessui.dev/react/listbox

Things that this PR implements:

Show/hide options on mouse click
Support for disabled component and individual options
Support for a optional label element
Full mouse support for focus and active elements
Full keyboard support (Enter/Space/ArrowUp/ArrowDown/ArrowLeft/ArrowRight/PageUp/PageDown/Home/End/Escape)
Support for searching the list using the keyboard
Click outside the listbox to close
Full accessibility implementation (aria-expanded/aria-labelledby/aria-controls/aria-haspopup/aria-active-descendent) and focus management
Sample implementation in dummy app

Things that this PR does not implement are:

static prop
Composition with transitions

I ported the React component tests for listbox as Ember integration tests. 90 integration tests were ported. Some tests were not ported because they are not applicable to Ember components. A test for transitions was not ported because transitions has not been implemented here yet.

😈 Before

Didn't exist.

😇 After

Closes #69

alexlafroscia

I'm really excited to see this PR!

I took a first pass through and left some thoughts; there is a lot to review here so it might take me a few passes.

I'm generally going to assume the test implementation is fine and that the tests passing are enough of an indication that they're good-to-go, and will instead focus on the actual add-on source code for review purposes (at least, for now!)

addon/components/listbox.hbs

addon/components/listbox.js

addon/components/listbox/-button.hbs

addon/components/listbox/-option.js

addon/components/listbox/-options.hbs

tests/integration/components/listbox-test.js

Use `isOpen` as the yielded property name Use a `setter` for this property to ensure state is correct when property is changed externally. Pass `openListbox` and `closeListbox` actions to components.

Use `focus-trap` to manage focus when opening/closing the listbox Use `click-outside` to manage document click events

Now storing the index of each listbox option within the list. This means we avoid an expensive `find` operation when setting the selected option on mouse click or key event.

dmcnamara-eng · 2021-09-01T15:48:44Z

I'm really excited to see this PR!

I took a first pass through and left some thoughts; there is a lot to review here so it might take me a few passes.

I'm generally going to assume the test implementation is fine and that the tests passing are enough of an indication that they're good-to-go, and will instead focus on the actual add-on source code for review purposes (at least, for now!)

Sounds good! Thanks for the quick feedback! I've replied to individual comments above.

addon/components/listbox.js

addon/components/listbox/-button.hbs

addon/components/listbox.hbs

addon/components/listbox/-option.js

addon/components/listbox.js

alexlafroscia · 2021-09-01T16:53:14Z

I approved the GitHub Actions run so we'll get lint/test results for the PR now. GitHub disables those for first-time contributors (people were abusing the system to mine cryptocurrency 🤦‍♂️) do you need a contributor to give the OK for CI to run on an individual's first PR!

dmcnamara-eng · 2021-09-02T10:20:07Z

Rather than using the click-outside modifier on the button, I think we should be able to use the onDeactivate option to close the menu and avoid needing to perform any focus-management-on-click-outside behavior ourselves.

660f5cf removes the click-outside modifier to use a pure focus-trap implementation.

I had to skip one test for this to pass. The value of that test seems dubious at this point.

dmcnamara-eng · 2021-09-02T10:48:03Z

I approved the GitHub Actions run so we'll get lint/test results for the PR now. GitHub disables those for first-time contributors (people were abusing the system to mine cryptocurrency 🤦‍♂️) do you need a contributor to give the OK for CI to run on an individual's first PR!

Those crazy cryptokids 😆

I've fixed the lint errors and all tests should be passing now, so please kick-off another CI and see how we are doing 👍

alexlafroscia

This is looking really great! Once we get clarity on the nested conditional I commented on, I am feeling good about merging this PR!

alexlafroscia · 2021-09-02T19:09:22Z

addon/components/listbox.js

+    return this._isOpen;
+  }
+
+  set isOpen(isOpen) {


I really like using a "setter" like this; keeps the code for manipulating the value clean, but allows us to perform these side-effects easily as well!

addon/components/listbox/-option.hbs

far-fetched · 2021-09-03T07:45:28Z

tests/accessibility-assertions.js

@@ -37,6 +61,12 @@ const DialogState = {
  InvisibleUnmounted: 'InvisibleUnmounted',
 };

+const ListboxState = {
+  Visible: 'Visible',
+  InvisibleHidden: 'InvisibleHidden',


I don't see use of this state in tests, I assume that we are just lack of some tests right ? Some assertions don't implement it. Do we have a plan to extend the test suite ?

Maybe would be nice to follow the same approach what @achambers did in dialog ? What I mean is copy test e.g. from react repo and change to todo example
I am willing to help with them after that and fill gaps.

Ah yes this refers to the alternative "hidden" or "unmount" render strategy, in which React will insert the listbox options into the DOM, but display as hidden.

I think it's safe to push this feature out for now.

b452c35 adds todos for these tests.

alexlafroscia

This is looking for to me! I'm happy to merge it at this point if you're ready @dmcnamara-eng !

dmcnamara-eng · 2021-09-03T16:03:00Z

This is looking for to me! I'm happy to merge it at this point if you're ready @dmcnamara-eng !

🥳 I'm happy. Please merge!

alexlafroscia · 2021-09-03T16:09:05Z

0.9.0 released with this PR in it! Awesome work here @dmcnamara-eng 🎉 💯

dmcnamara-eng · 2021-09-03T16:13:39Z

0.9.0 released with this PR in it! Awesome work here @dmcnamara-eng 🎉 💯

Cheers. Happy days. Let's get this library to1.0.0!

GavinJoyce · 2021-09-03T16:24:46Z

❤️

miguelcobain · 2022-11-16T22:28:35Z

@dmcnamara-eng how difficult would it be to implement the multiple feature on headlessui? https://headlessui.com/react/listbox#selecting-multiple-values

knownasilya · 2022-11-19T23:03:35Z

This renders in place instead of absolutely positioned in the body?

dmcnamara-eng added 5 commits August 31, 2021 15:08

📦 Use ember-sinon

c3462f7

✅ Implement <Listbox> tests

fd11cf7

✨ Implement <Listbox> component

b49a6f0

✨ Add basic <Listbox> to dummy app

86edb5d

✨ Stub components/modifiers in app

c173a83

dmcnamara-eng changed the title ~~✨ Introduce <Listbox /> component~~ ✨ Implement <Listbox /> component Aug 31, 2021

alexlafroscia reviewed Aug 31, 2021

View reviewed changes

dmcnamara-eng added 5 commits September 1, 2021 16:43

♻️ Use isOpen and open setter

f80e94f

Use `isOpen` as the yielded property name Use a `setter` for this property to ensure state is correct when property is changed externally. Pass `openListbox` and `closeListbox` actions to components.

♻️ Use focus-trap and click-outside

8fb0a7f

Use `focus-trap` to manage focus when opening/closing the listbox Use `click-outside` to manage document click events

♻️ Use isDisabled property

4eca07a

♻️ Allow custom ul tagname

2489d11

♻️ Improve performance when setting a selected option

9779e56

Now storing the index of each listbox option within the list. This means we avoid an expensive `find` operation when setting the selected option on mouse click or key event.

dmcnamara-eng added 2 commits September 1, 2021 17:17

♻️ Remove ember-sinon dependency

dac162c

♻️ Remove 1px class from listbox

572c5e0

alexlafroscia reviewed Sep 1, 2021

View reviewed changes

alexlafroscia added the enhancement New feature or request label Sep 1, 2021

dmcnamara-eng added 5 commits September 2, 2021 10:17

♻️ Expose openListbox, closeListbox, isOpen

45a2f57

♻️ Remove aria modifier

11c9843

♻️ Option GUID cleanup

2c8811d

♻️ find -> forEach

03e6222

♻️ Remove click-outside dependency

660f5cf

🚨 Fix linter errors

38d84c9

dmcnamara-eng marked this pull request as ready for review September 2, 2021 10:48

dmcnamara-eng requested a review from alexlafroscia September 2, 2021 10:48

alexlafroscia reviewed Sep 2, 2021

View reviewed changes

far-fetched reviewed Sep 3, 2021

View reviewed changes

dmcnamara-eng added 2 commits September 3, 2021 09:49

♻️ Remove redundant conditional

bdce10e

♻️ Add test todos for hidden render strategy

b452c35

alexlafroscia reviewed Sep 3, 2021

View reviewed changes

alexlafroscia approved these changes Sep 3, 2021

View reviewed changes

alexlafroscia merged commit 653a407 into GavinJoyce:master Sep 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Implement `<Listbox />` component #76

✨ Implement `<Listbox />` component #76

dmcnamara-eng commented Aug 31, 2021 •

edited

alexlafroscia left a comment •

edited

dmcnamara-eng commented Sep 1, 2021

alexlafroscia commented Sep 1, 2021

dmcnamara-eng commented Sep 2, 2021

dmcnamara-eng commented Sep 2, 2021

alexlafroscia left a comment

alexlafroscia Sep 2, 2021

far-fetched Sep 3, 2021

far-fetched Sep 3, 2021

dmcnamara-eng Sep 3, 2021

alexlafroscia left a comment

dmcnamara-eng commented Sep 3, 2021

alexlafroscia commented Sep 3, 2021

dmcnamara-eng commented Sep 3, 2021

GavinJoyce commented Sep 3, 2021

miguelcobain commented Nov 16, 2022

knownasilya commented Nov 19, 2022

✨ Implement <Listbox /> component #76

✨ Implement <Listbox /> component #76

Conversation

dmcnamara-eng commented Aug 31, 2021 • edited

✨ What Changed and Why

😈 Before

😇 After

alexlafroscia left a comment • edited

Choose a reason for hiding this comment

dmcnamara-eng commented Sep 1, 2021

alexlafroscia commented Sep 1, 2021

dmcnamara-eng commented Sep 2, 2021

dmcnamara-eng commented Sep 2, 2021

alexlafroscia left a comment

Choose a reason for hiding this comment

alexlafroscia Sep 2, 2021

Choose a reason for hiding this comment

far-fetched Sep 3, 2021

Choose a reason for hiding this comment

far-fetched Sep 3, 2021

Choose a reason for hiding this comment

dmcnamara-eng Sep 3, 2021

Choose a reason for hiding this comment

alexlafroscia left a comment

Choose a reason for hiding this comment

dmcnamara-eng commented Sep 3, 2021

alexlafroscia commented Sep 3, 2021

dmcnamara-eng commented Sep 3, 2021

GavinJoyce commented Sep 3, 2021

miguelcobain commented Nov 16, 2022

knownasilya commented Nov 19, 2022

✨ Implement `<Listbox />` component #76

✨ Implement `<Listbox />` component #76

dmcnamara-eng commented Aug 31, 2021 •

edited

alexlafroscia left a comment •

edited