-
Notifications
You must be signed in to change notification settings - Fork 601
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
use Floating UI for Select and Combobox #6452
Conversation
7b5e781
to
73bfd9b
Compare
Is it unreasonable for someone so inclined to try and port this change to |
Let me consider the implications of the change - I’m not sure it would fall under what we would consider “non-breaking”, but open to considering it. |
@atmgrifter00 This same method could be added to a v1 select by extending the export class MyComponent extends Select {
public cleanup: () => void;
/**
* Overrides the `setPositioning()` method in Select.
* This breaks functionality for the `positionAttribute`, `position`, `forcedPosition`, and `maxHeight` properties.
*/
public setPositioning() {
if (this.$fastController.isConnected) {
this.cleanup = autoUpdate(this, this.listbox, async () => {
const { middlewareData, x, y } = await computePosition(
this.control,
this.listbox,
{
placement: "bottom",
strategy: "fixed",
middleware: [
flip(),
size({
apply: ({ availableHeight, rects }) => {
Object.assign(this.listbox.style, {
maxHeight: `${availableHeight}px`,
width: `${rects.reference.width}px`,
});
},
}),
hide(),
],
}
);
if (middlewareData.hide?.referenceHidden) {
this.open = false;
return;
}
Object.assign(this.listbox.style, {
position: "fixed",
top: "0",
left: "0",
transform: `translate(${x}px, ${y}px)`,
});
});
}
}
/**
* overrides the `openChanged()` method in Select.
* `this.setPositioning()` has to be called on the next tick so the position is measured correctly.
*/
protected openChanged(prev: boolean | undefined, next: boolean): void {
if (!this.collapsible) {
return;
}
if (this.open) {
this.ariaControls = this.listboxId;
this.ariaExpanded = "true";
Updates.enqueue(() => this.setPositioning());
this.focusAndScrollOptionIntoView();
this.indexWhenOpened = this.selectedIndex;
// focus is directed to the element when `open` is changed programmatically
Updates.enqueue(() => this.focus());
return;
}
this.cleanup?.();
this.ariaControls = "";
this.ariaExpanded = "false";
}
// overrides the `disconnectedCallback()` method in Select
public disconnectedCallback() {
this.cleanup?.();
super.disconnectedCallback();
}
} |
this.cleanup = autoUpdate(this, this.listbox, async () => { | ||
const { middlewareData, x, y } = await computePosition( | ||
this, | ||
this.listbox, | ||
{ | ||
placement: "bottom", | ||
strategy: "fixed", | ||
middleware: [ | ||
flip(), | ||
size({ | ||
apply: ({ availableHeight, rects }) => { | ||
Object.assign(this.listbox.style, { | ||
maxHeight: `${availableHeight}px`, | ||
width: `${rects.reference.width}px`, | ||
}); | ||
}, | ||
}), | ||
hide(), | ||
], | ||
} | ||
); | ||
|
||
if (middlewareData.hide?.referenceHidden) { | ||
this.open = false; | ||
this.cleanup(); | ||
return; | ||
} | ||
|
||
Object.assign(this.listbox.style, { | ||
position: "fixed", | ||
top: "0", | ||
left: "0", | ||
transform: `translate(${x}px, ${y}px)`, | ||
}); | ||
}); |
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'd love to see us look into abstracting this into a utility which could be shared rather than repeated once we have a good baseline for implementation.
863a0df
to
b3e6aa6
Compare
83b98db
to
e2a71fb
Compare
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.
Glad to see this moving along! I'll get the other PRs reviewed either later tonight or first thing in the morning. Thanks @radium-v for helping to resolve this!
change/@microsoft-fast-foundation-7f1045f7-4da2-43db-868c-86a3472b38da.json
Outdated
Show resolved
Hide resolved
@@ -328,6 +301,11 @@ export class FASTCombobox extends FormAssociatedCombobox { | |||
this.ariaDisabled = this.disabled ? "true" : "false"; | |||
} | |||
|
|||
public disconnectedCallback(): void { | |||
this.cleanup?.(); |
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.
Nice use of the operator here!
placement: "bottom", | ||
strategy: "fixed", | ||
middleware: [ | ||
flip(), |
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.
Am I correct in assuming that though is is positioned to the bottom, if there's not enough space, this will "flip" it to the top?
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.
Yep, that's what this middleware does: https://floating-ui.com/docs/flip
@observable | ||
public maxHeight: number = 0; | ||
public setPositioning(): void { | ||
if (this.$fastController.isConnected) { |
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.
Bit of duplication here with the combo. Is it worth factoring out a shared function?
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.
There's a lot of crossover between listbox, select, and combobox. They all share the same common abstract FASTListbox
, but at different stages. Here are the functional differences that split them into certain categories:
Detail | abstract FASTListbox |
FASTListboxElement |
FASTSelect |
FASTCombobox |
---|---|---|---|---|
Class declaration | extends FASTElement |
extends FASTListbox |
extends FASTListboxElement |
extends FASTListbox |
multiple attribute |
❌ | ✅ | ✅ | ❌ |
size attribute |
❌ | ✅ | ✅ | ❌ |
value attribute |
❌ | ❌ | ✅ | ✅ |
collapsible | N/A | ❌ | in single-selection mode, without size |
always |
proxy element | N/A | N/A | <select> |
<input> |
I'll make a separate issue to track this, since wouldn't be as simple as just putting the setPositioning()
method in the abstract FASTListbox
class.
e2a71fb
to
5f929e7
Compare
5f929e7
to
3904dda
Compare
3904dda
to
188117f
Compare
* remove position attribute and related properties from combobox * use floating-ui for combobox listbox * remove position attribute and related properties from select * use floating-ui for select listbox * Change files
Pull Request
📖 Description
Replaces the methods used to position the options list for combobox and select with Floating UI.
Removes the
positionAttribute
,position
, andmaxHeight
properties from select and combobox.🎫 Issues
closes #5629.
closes #4971
related to #5363
related to #6185
closes #6373
👩💻 Reviewer Notes
Due to an issue in Storybook (storybookjs/storybook#16774), I added a style to disable the
transform
property on a parent div for the docs preview. This mitigates a bug in Firefox which prevents the listbox from escaping the demo container, but it also breaks SB's zoom control button functionality. Completely removing the zoom controls would require building a custom docs page.📑 Test Plan
All tests for combobox and select should pass as expected.
✅ Checklist
General
$ yarn change
Component-specific