Split Select dual popover/listbox element in two #1789
Conversation
acelaya
force-pushed
the
select-explicit-popover
branch
from
cdfe295 to
701fe9c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1789 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 67 67
Lines 1204 1203 -1
Branches 455 455
=========================================
- Hits 1204 1203 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
acelaya
force-pushed
the
select-explicit-popover
branch
3 times, most recently
from
4e705bc to
e655188
Compare
| */ | ||
| function useListboxPositioning( | ||
| function usePopoverPositioning( |
There was a problem hiding this comment.
I removed all references to "listbox" from this hook, because it is likely going to be extracted and decoupled from that.
acelaya
force-pushed
the
select-explicit-popover
branch
from
e655188 to
87fe3a5
Compare
| @@ -257,16 +257,16 @@ function SelectOption<T>({ | |||
|
|
|||
| SelectOption.displayName = 'Select.Option'; | |||
|
|
|||
| /** Small space to apply between the toggle button and the listbox */ | |||
There was a problem hiding this comment.
This PR is full of listbox/popover replacements.
| .getDOMNode() | ||
| .getBoundingClientRect(); | ||
|
|
||
| assert.isTrue(listboxWidth < window.innerWidth); | ||
| assert.isBelow(popoverWidth, window.innerWidth); |
There was a problem hiding this comment.
assert.isBelow(a, b) generates a better error than assert.isTrue(a < b) if the assertion fails.
acelaya
force-pushed
the
select-explicit-popover
branch
from
87fe3a5 to
1018f95
Compare
| // We don't want the listbox to ever render outside the viewport, | ||
| // and we give it a 16px gap | ||
| 'max-w-[calc(100%-16px)]', | ||
| // Overwrite [popover] default styles |
There was a problem hiding this comment.
What are the popover default styles?
There was a problem hiding this comment.
It sets margin: auto so that popovers are horizontally and vertically centered in the top layer. Unfortunately, it messes the positioning calculation.
It also sets padding: 0.25rem, which adds-up to the manually set padding in the listbox.
Surprisingly, it didn't have any effect when [popover] was used in a ul element. I suspect it was because of the style resets that tailwind sets.
There was a problem hiding this comment.
Surprisingly, it didn't have any effect when
[popover]was used in aulelement. I suspect it was because of the style resets that tailwind sets.
Yep, exactly.
| aria-orientation="vertical" | ||
| data-testid="select-listbox" | ||
| data-listbox-open={listboxOpen} | ||
| ref={popoverRef} | ||
| // nb. Use `undefined` rather than `false` because Preact doesn't | ||
| // handle boolean values correctly for this attribute (it will set | ||
| // `popover="false"` instead of removing the attribute). |
There was a problem hiding this comment.
Incidentally I think this might be fixed now, but it needs testing.
There was a problem hiding this comment.
Yep. I considered changing it, but I think we can tackle it individually
This PR is the first of a series aiming to simplify/extract capabilities from the
Selectcomponent.Here we are splitting the listbox-as-popover element into two independent elements:
divwhich acts as the popover, using the popover API when possible, and handling positioning and openness logic.ulwith the listbox role, which handles accessibility aspects to handle a list of items, like aria attributes and keyboard navigation. It is the only children of the popover element.For now, this split is done inside of the
Selectcomponent, keeping it unchanged from a consumer or user point of view.In follow-up PRs we'll we extracting some/all of those components.
Note
This PR renames many references to the
listboxconcept topopover, but there are a few grey areas that I left unchanged.I expect some of those to be changed in upcoming PRs, where it will become more obvious as we start extracting sub-components.
Tip
This PR is easier to review ignoring whitespaces
Testing steps
Sanity-check that everything in http://localhost:4001/input-select looks and behaves the same as before this change.