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.

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

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

Merged
merged 8 commits into from
Dec 13, 2023

Conversation

driskull
Copy link
Member

@driskull driskull commented Dec 12, 2023

Related Issue: #7979 #8214 #8386 #8419 #5697

Summary

@github-actions github-actions bot added the chore Issues with changes that don't modify src or test files. label Dec 12, 2023
@@ -489,8 +494,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.

@driskull driskull changed the title revert: fix: floating components will now get an initial position even if they are not opened fix(floating-ui): improve floating element performance. Dec 12, 2023
@driskull driskull changed the title fix(floating-ui): improve floating element performance. fix(floating-ui): improve floating element performance Dec 12, 2023
@driskull driskull marked this pull request as ready for review December 12, 2023 23:08
@driskull driskull requested a review from a team as a code owner December 12, 2023 23:08
@driskull driskull added 2 - in development Issues that are actively being worked on. pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Dec 12, 2023
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Dec 13, 2023
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Dec 13, 2023
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎
🏎🎈🎈🎈🎈🏎🎈🏎🏎🏎🏎🏎🎈🏎🎈🎈🎈🎈🏎🏎🎈🎈🎈🏎🏎🎈🎈🏎🏎🎈🏎🏎🏎🎈🏎🎈🎈🎈🎈🏎
🏎🎈🏎🏎🎈🏎🎈🏎🏎🏎🏎🏎🎈🏎🎈🏎🏎🏎🏎🎈🏎🏎🏎🏎🎈🏎🏎🎈🏎🎈🎈🏎🎈🎈🏎🎈🏎🏎🏎🏎
🏎🎈🎈🎈🎈🏎🎈🏎🏎🎈🏎🏎🎈🏎🎈🎈🎈🏎🏎🏎🎈🎈🏎🏎🎈🏎🏎🎈🏎🎈🏎🎈🏎🎈🏎🎈🎈🎈🏎🏎
🏎🎈🏎🏎🎈🏎🎈🏎🎈🏎🎈🏎🎈🏎🎈🏎🏎🏎🏎🏎🏎🏎🎈🏎🎈🏎🏎🎈🏎🎈🏎🏎🏎🎈🏎🎈🏎🏎🏎🏎
🏎🎈🏎🏎🎈🏎🏎🎈🏎🏎🏎🎈🏎🏎🎈🎈🎈🎈🏎🎈🎈🎈🏎🏎🏎🎈🎈🏎🏎🎈🏎🏎🏎🎈🏎🎈🎈🎈🎈🏎
🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎🏎

Do we have a perf impact issue we could link to for this PR?

@@ -489,8 +494,6 @@ export function connectFloatingUI(

// initial positioning based on https://floating-ui.com/docs/computePosition#initial-layout
position: component.overlayPositioning,
top: "0",
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.

@driskull
Copy link
Member Author

Do we have a perf impact issue we could link to for this PR?

No, can we get one? cc @paulcpederson

@jcfranco
Copy link
Member

@driskull Could you also check if this introduces a regression for #5697? 🙇‍♂️

@paulcpederson
Copy link
Member

#8419

@driskull
Copy link
Member Author

@driskull Could you also check if this introduces a regression for #5697? 🙇‍♂️

@jcfranco It seems like this would be a regression here. This makes sense because the component doesn't get its position necessary for a proper animation until after its opened. Once it has been opened once the position is there and the correct animation takes place.

I think we'll have to accept this for now. Thoughts?

@jcfranco jcfranco removed the chore Issues with changes that don't modify src or test files. label Dec 13, 2023
@jcfranco
Copy link
Member

I think we'll have to accept this for now. Thoughts?

Agreed. Let's reopen the issue and comment on the reasoning. cc @geospatialem

@driskull driskull merged commit 4d8cfb8 into main Dec 13, 2023
20 checks passed
@driskull driskull deleted the dris0000/revert-8001 branch December 13, 2023 19:49
@github-actions github-actions bot added this to the 2023 December Priorities milestone Dec 13, 2023
benelan pushed a commit that referenced this pull request Dec 19, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>@esri/calcite-design-tokens: 2.1.0</summary>

##
[2.1.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2023-12-19)


### Features

* **font tokens:** Add font family fallbacks
([#8389](#8389))
([b2fd255](b2fd255))
* Update tokens for better documentation
([#8395](#8395))
([ff19630](ff19630))


### Bug Fixes

* **color tokens:** Fix errors in schema
([#8446](#8446))
([f36a90e](f36a90e))
* **color-context tokens:** Only apply when `.calcite-mode-auto` is
applied
([#8344](#8344))
([19de817](19de817))
* Replace "\n" to support Windows for tokens output
([#8352](#8352))
([02cf5d5](02cf5d5))
</details>

<details><summary>@esri/calcite-components: 2.1.0</summary>

##
[2.1.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2023-12-19)


### Features

* Add `validation-message` and `validation-icon` attributes to form
components for new internal `calcite-input-message`
([#8305](#8305))
([a554cfd](a554cfd))
* **dropdown-item:** Add disabled support
([#8312](#8312))
([4c311c6](4c311c6))
* **list-item:** Add calciteListItemToggle event.
([#8433](#8433))
([1d2fa04](1d2fa04))
* **list:** Add `calciteListDragStart` and `calciteListDragEnd` events
for when a drag starts and ends.
([#8361](#8361))
([1314605](1314605))
* **list:** Add drag event details to `calciteListDragStart` and
`calciteListDragEnd` events
([#8438](#8438))
([e199c08](e199c08))
* Provide legacy CSS custom props for backwards compatibility
([#8355](#8355))
([b0f063e](b0f063e))


### Bug Fixes

* **action-menu:** Filter hidden or disabled actions via keyboard.
([#8336](#8336))
([11c0007](11c0007))
* **action-menu:** Fix closing action menu after a drag occurs
([#8339](#8339))
([dcb8548](dcb8548))
* **action-menu:** Keep internal popover open property in sync
([#8387](#8387))
([38dff7c](38dff7c))
* **dropdown-item:** Avoid hover/active styling when disabled
([#8398](#8398))
([35817dc](35817dc))
* **floating-ui:** Improve floating element performance
([#8409](#8409))
([4d8cfb8](4d8cfb8))
* **input-date-picker:** Ensure range input toggling is consistent
([#8414](#8414))
([cd92586](cd92586))
* **input-date-picker:** No longer emits redundant change event
([#8341](#8341))
([cd5b92b](cd5b92b))
* **input-date-picker:** Respect the numberingSystem property when
rendering the input
([#8383](#8383))
([395b538](395b538))
* **list-item:** Drag grid cell should be accessible via arrow keys.
([#8353](#8353))
([2718ab3](2718ab3))
* **menu-item:** Improve keyboard navigability when `href` populated
([#8408](#8408))
([5b44798](5b44798))
* **modal:** Ensure document overflow styles are properly restored when
multiple modals are closed/removed
([#8390](#8390))
([f2c6b09](f2c6b09))
* Replace "\n" to support Windows for tokens output
([#8352](#8352))
([02cf5d5](02cf5d5))
* **shell-panel:** Adds border at the start when slotted in `panel-end`
([#8314](#8314))
([2d1a1e2](2d1a1e2))
* **shell, shell-panel:** Support resizing shell panel when there is an
iframe slotted in shell content
([#8317](#8317))
([e0f69c9](e0f69c9))
* **stepper:** Emits `calciteStepperItemChange` event when switched to
first step
([#8422](#8422))
([508979f](508979f))
* **table-cell:** Fix background css variable
([#8439](#8439))
([9e5c59b](9e5c59b))
* **tab:** Prevent vertical scrollbar on content pane when the height of
outer elements are specified
([#8399](#8399))
([9e6d901](9e6d901))
* Use Stencil watchers instead of global attributes util
([#8407](#8407))
([c531d81](c531d81))


### Dependencies

* The following workspace dependencies were updated
  * devDependencies
    * @esri/calcite-design-tokens bumped from ^2.1.0-next.1 to ^2.1.0
</details>

<details><summary>@esri/calcite-components-angular: 2.1.0</summary>

##
[2.1.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2023-12-19)


### Miscellaneous Chores

* **@esri/calcite-components-angular:** Synchronize undefined versions


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^2.1.0-next.12 to ^2.1.0
</details>

<details><summary>@esri/calcite-components-react: 2.1.0</summary>

##
[2.1.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2023-12-19)


### Miscellaneous Chores

* **@esri/calcite-components-react:** Synchronize undefined versions


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^2.1.0-next.12 to ^2.1.0
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - in development Issues that are actively being worked on. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants