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

Guarantee DOM sort order when performing actions #1168

Merged
merged 2 commits into from
Feb 28, 2022
Merged

Guarantee DOM sort order when performing actions #1168

merged 2 commits into from
Feb 28, 2022

Commits on Feb 28, 2022

  1. ensure proper sort order 

    We already fixed a bug in the past where the order of DOM nodes wasn't
stored in the correct order when performing operations (e.g.: using your
keyboard to go to the next option).

We fixed this by ensuring that when we register/unregister an
option/item, that we sorted the list properly. This worked fine, until
we introduced the Combobox components. This is because items in a
Combobox are continuously filtered and because of that moved around.

Moving a DOM node to a new position _doesn't_ require a full
unmount/remount. This means that the sort gets messed up and the order
is wrong when moving around again.

To fix this, we will always perform a sort when performing actions. This
could have performance drawbacks, but the alternative is to re-sort when
the component gets updated. The bad part is that you can update a
component via many ways (like changes on the parent), in those
scenario's you probably don't care to properly re-order the internal
list. Instead we do it while performing an action (`goToOption` / `goToItem`).

To make things a bit more efficient, instead of querying the DOM all the
time using `document.querySelectorAll`, we will keep track of the
underlying DOM node instead. This does increase memory usage a bit but I
think that this is a fine trade-off.

Performance wise this could also be a bottleneck to perform the sorting
if you have a lot of data. But this problem already exists today,
therefore I consider this a complete new problem instead to solve. Maybe
we don't solve it in Headless UI itself, but figure out a way to make it
composable with existing virtualization libraries.
    @RobinMalfait
    RobinMalfait committed Feb 28, 2022
    Configuration menu
    Copy the full SHA
    f897b0b View commit details
    Browse the repository at this point in the history

  2. update changelog

    @RobinMalfait
    RobinMalfait committed Feb 28, 2022
    Configuration menu
    Copy the full SHA
    8231843 View commit details
    Browse the repository at this point in the history