Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(floating-ui): improve floating element performance #8409

Merged
merged 8 commits into from
Dec 13, 2023
Merged

fix(floating-ui): improve floating element performance #8409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e544969
revert: fix: floating components will now get an initial position eve…
driskull Dec 12, 2023
bef79b8
cleanup
driskull Dec 12, 2023
6d7268f
fix tests
driskull Dec 12, 2023
371a1eb
cleanup
driskull Dec 12, 2023
beda1b7
cleanup
driskull Dec 12, 2023
4f6853b
Merge branch 'main' into dris0000/revert-8001
driskull Dec 12, 2023
2517338
remove max-content
driskull Dec 13, 2023
054d20c
Merge branch 'main' into dris0000/revert-8001
driskull Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/calcite-components/src/assets/styles/_sortable.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
@mixin sortable-helper-classes() {
.calcite-sortable--chosen,
.calcite-sortable--ghost,
.calcite-sortable--drag {
overflow: hidden;
Expand Down
10 changes: 10 additions & 0 deletions packages/calcite-components/src/components.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6260,6 +6260,8 @@ declare global {
};
interface HTMLCalciteListElementEventMap {
"calciteListChange": void;
"calciteListDragEnd": ListDragDetail;
"calciteListDragStart": ListDragDetail;
"calciteListFilter": void;
"calciteListOrderChange": ListDragDetail;
"calciteInternalListDefaultSlotChange": void;
Expand Down Expand Up @@ -9984,6 +9986,14 @@ declare namespace LocalJSX {
* Emits when any of the list item selections have changed.
*/
"onCalciteListChange"?: (event: CalciteListCustomEvent<void>) => void;
/**
* Emits when the component's dragging has ended.
*/
"onCalciteListDragEnd"?: (event: CalciteListCustomEvent<ListDragDetail>) => void;
/**
* Emits when the component's dragging has started.
*/
"onCalciteListDragStart"?: (event: CalciteListCustomEvent<ListDragDetail>) => void;
/**
* Emits when the component's filter has changed.
*/
Expand Down
10 changes: 5 additions & 5 deletions packages/calcite-components/src/components/dropdown/dropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,14 @@ export class Dropdown

@Watch("open")
openHandler(): void {
if (!this.disabled) {
onToggleOpenCloseComponent(this);
onToggleOpenCloseComponent(this);

if (this.disabled) {
this.open = false;
return;
}

this.open = false;
this.reposition(true);
driskull marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -549,7 +551,6 @@ export class Dropdown
};

onBeforeOpen(): void {
this.reposition(true);
this.calciteDropdownBeforeOpen.emit();
}

Expand All @@ -563,7 +564,6 @@ export class Dropdown

onClose(): void {
this.calciteDropdownClose.emit();
this.reposition(true);
}

setReferenceEl = (el: HTMLDivElement): void => {
Expand Down
4 changes: 2 additions & 2 deletions packages/calcite-components/src/components/input-date-picker/input-date-picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,8 @@ export class InputDatePicker
this.open = false;
return;
}

this.reposition(true);
}

/**
Expand Down Expand Up @@ -766,7 +768,6 @@ export class InputDatePicker
}

onBeforeOpen(): void {
this.reposition(true);
this.calciteInputDatePickerBeforeOpen.emit();
}

Expand All @@ -792,7 +793,6 @@ export class InputDatePicker
this.restoreInputFocus();
this.focusOnOpen = false;
this.datePickerEl.reset();
this.reposition(true);
}

setStartInput = (el: HTMLCalciteInputElement): void => {
Expand Down
7 changes: 3 additions & 4 deletions packages/calcite-components/src/components/input-time-picker/input-time-picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,16 +172,15 @@ export class InputTimePicker
@Prop({ reflect: true, mutable: true }) open = false;

@Watch("open")
openHandler(open: boolean): void {
openHandler(): void {
onToggleOpenCloseComponent(this);

if (this.disabled || this.readOnly) {
this.open = false;
return;
}

if (open) {
this.reposition(true);
}
this.reposition(true);
}

/** When `true`, interaction is prevented and the component is displayed with lower opacity. */
Expand Down
4 changes: 1 addition & 3 deletions packages/calcite-components/src/components/popover/popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export class Popover
@Watch("open")
openHandler(): void {
onToggleOpenCloseComponent(this);

this.reposition(true);
this.setExpandedAttr();
}

Expand Down Expand Up @@ -499,7 +499,6 @@ export class Popover
};

onBeforeOpen(): void {
this.reposition(true);
this.calcitePopoverBeforeOpen.emit();
}

Expand All @@ -515,7 +514,6 @@ export class Popover
onClose(): void {
this.calcitePopoverClose.emit();
deactivateFocusTrap(this);
this.reposition(true);
}

storeArrowEl = (el: SVGElement): void => {
Expand Down
3 changes: 1 addition & 2 deletions packages/calcite-components/src/components/tooltip/tooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ export class Tooltip implements FloatingUIComponent, OpenCloseComponent {
@Watch("open")
openHandler(): void {
onToggleOpenCloseComponent(this);
this.reposition(true);
}

/**
Expand Down Expand Up @@ -249,7 +250,6 @@ export class Tooltip implements FloatingUIComponent, OpenCloseComponent {
// --------------------------------------------------------------------------

onBeforeOpen(): void {
this.reposition(true);
this.calciteTooltipBeforeOpen.emit();
}

Expand All @@ -263,7 +263,6 @@ export class Tooltip implements FloatingUIComponent, OpenCloseComponent {

onClose(): void {
this.calciteTooltipClose.emit();
this.reposition(true);
}

private setTransitionEl = (el): void => {
Expand Down
2 changes: 1 addition & 1 deletion packages/calcite-components/src/tests/commonTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1352,7 +1352,7 @@ export function floatingUIOwner(
await scrollTo(scrollablePageSizeInPx, scrollablePageSizeInPx);
await page.waitForChanges();

expect(await getTransform()).not.toBe(initialClosedTransform);
expect(await getTransform()).toBe(initialClosedTransform);

await scrollTo(0, 0);
await page.waitForChanges();
Expand Down
16 changes: 13 additions & 3 deletions packages/calcite-components/src/utils/floating-ui.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,24 +80,34 @@ describe("repositioning", () => {
expect(floatingEl.style.left).toBe("0");
}

it("repositions immediately by default", async () => {
it("repositions only for open components", async () => {
await reposition(fakeFloatingUiComponent, positionOptions);
assertPreOpenPositioning(floatingEl);

fakeFloatingUiComponent.open = true;

await reposition(fakeFloatingUiComponent, positionOptions);
assertOpenPositioning(floatingEl);
});

it("repositions immediately by default", async () => {
fakeFloatingUiComponent.open = true;

reposition(fakeFloatingUiComponent, positionOptions);

assertPreOpenPositioning(floatingEl);

await waitForAnimationFrame();
assertOpenPositioning(floatingEl);
});

it("can reposition after a delay", async () => {
assertPreOpenPositioning(floatingEl);

fakeFloatingUiComponent.open = true;

reposition(fakeFloatingUiComponent, positionOptions, true);

assertPreOpenPositioning(floatingEl);

await new Promise<void>((resolve) => setTimeout(resolve, repositionDebounceTimeout));
assertOpenPositioning(floatingEl);
});
Expand Down
10 changes: 6 additions & 4 deletions packages/calcite-components/src/utils/floating-ui.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,8 @@ export const positionFloatingUI =
pointerEvents,
position,
transform: open ? `translate(${roundByDPR(x)}px,${roundByDPR(y)}px)` : "",
left: open ? "0" : "",
top: open ? "0" : "",
top: 0,
left: 0,
});
};

Expand Down Expand Up @@ -427,6 +427,10 @@ export async function reposition(
options: Parameters<typeof positionFloatingUI>[1],
delayed = false
): Promise<void> {
if (!component.open) {
return;
}

const positionFunction = delayed ? getDebouncedReposition(component) : positionFloatingUI;

return positionFunction(component, options);
Expand Down Expand Up @@ -489,8 +493,6 @@ export function connectFloatingUI(

// initial positioning based on https://floating-ui.com/docs/computePosition#initial-layout
position: component.overlayPositioning,
top: "0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcfranco do you know why this was here?

It was added here: e220686#diff-3c419c81de1cc684c4d1d859881782bc87d089da410675b5af3f6c468b95ef13R458

I'd like to remove this to fix the drag and drop positioning issue. #7979

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, this is only set once an actual position occurs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's based on the Floating UI computedPosition doc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that doc says only the position needs to be there initially so the top and left can happen later.

left: "0",
});

const runAutoUpdate = Build.isBrowser
Expand Down
Loading