refactor: replace iron-list with virtualizer in combo-box #2339
Conversation
tomivirkki
force-pushed
the
combo-box-virtualizer
branch
3 times, most recently
from
7fedaed
to
4f5a33c
Compare
packages/vaadin-combo-box/src/vaadin-combo-box-dropdown-wrapper.js
Outdated
Show resolved
Hide resolved
packages/vaadin-combo-box/src/vaadin-combo-box-dropdown-wrapper.js
Outdated
Show resolved
Hide resolved
tomivirkki
force-pushed
the
combo-box-virtualizer
branch
2 times, most recently
from
4b499b0
to
9584027
Compare
tomivirkki
force-pushed
the
combo-box-virtualizer
branch
from
9584027
to
f5c53c2
Compare
| if (this._isNotEmpty(items) && this._selector && !this.filterChanged) { | ||
| // iron-list triggers the scroller's reset after items update, and | ||
| // this is not appropriate for undefined size lazy loading. | ||
| // see https://github.com/vaadin/vaadin-combo-box-flow/issues/386 | ||
| // We store iron-list scrolling position in order to restore | ||
| // it later on after the items have been updated. | ||
| const currentScrollerPosition = this._selector.firstVisibleIndex; | ||
| if (currentScrollerPosition !== 0) { | ||
| this._oldScrollerPosition = currentScrollerPosition; | ||
| this._resetScrolling = true; | ||
| } | ||
| } | ||
| // Let the position to be restored in the future calls unless it's not | ||
| // caused by filtering | ||
| this.filterChanged = false; | ||
| return items; | ||
| } | ||
| return []; | ||
| } | ||
|
|
||
| _restoreScrollerPosition(items) { | ||
| if (this._isNotEmpty(items) && this._selector && this._oldScrollerPosition !== 0) { | ||
| // new items size might be less than old scrolling position | ||
| this._scrollIntoView(Math.min(items.length - 1, this._oldScrollerPosition)); | ||
| this._resetScrolling = false; | ||
| // reset position to 0 again in order to properly handle the filter | ||
| // cases (scroll to 0 after typing the filter) | ||
| this._oldScrollerPosition = 0; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
All logic related to restoring the scroll position on items change can be removed from combo-box, because the virtualizer has the same functionality built-in.
packages/vaadin-combo-box/src/vaadin-combo-box-dropdown-wrapper.js
Outdated
Show resolved
Hide resolved
packages/vaadin-combo-box/src/vaadin-combo-box-dropdown-wrapper.js
Outdated
Show resolved
Hide resolved
|
Let's also update We don't have visual tests for opened combo-box so they didn't catch this 😕 My bad, forgot to add them. |
Good catch, fixed |
|
Unit tests started to fail after the CSS change which makes me think they should be updated to not use Lumo. |
Yeah, that's probably a nicer fix. The numbers make better sense without Lumo, but it looks nasty which can make it difficult to debug the tests later. |
|
Kudos, SonarCloud Quality Gate passed!
|
|
This ticket/PR has been released with platform 22.0.0.alpha3 and is also targeting the upcoming stable 22.0.0 version. |
Replaces the Polymer-based
<iron-list>used for the dropdown items virtual scrolling with the internal virtualizer from<vaadin-virtual-list>.Certain Flow
ComboBoxtests and the TB element still expect the<vaadin-combo-box>Web Component to use an<iron-list>internally. There's a branch with necessary changes toComboBoxthat needs to be merged together with the Web Component version bump.