refactor: make combo-box use DataProviderController #7044
Conversation
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
from
3fbad4a
to
b3e1d14
Compare
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
from
e96006a
to
223e39f
Compare
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
from
5e2e3bf
to
ab03e1e
Compare
|
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
from
ab03e1e
to
34ecc64
Compare
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
6 times, most recently
from
1418504
to
f08d006
Compare
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
from
73cc8c8
to
9957d9c
Compare
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
4 times, most recently
from
bd70e58
to
065f12b
Compare
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
3 times, most recently
from
630a74a
to
d8a450b
Compare
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
from
33d13ba
to
8963067
Compare
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
2 times, most recently
from
fcbea2c
to
9e4b5e2
Compare
vursen
changed the title
| Object.keys(this.pendingRequests).forEach((page) => { | ||
| const startIndex = parseInt(page) * this.pageSize; | ||
| if (startIndex >= this.size || 0) { | ||
| delete this.pendingRequests[page]; | ||
| } | ||
| }); |
There was a problem hiding this comment.
note: This change also affects grid, but it makes sense there as well.
| // The controller adds new items to the cache through mutation, | ||
| // so we need to create a new array to trigger filteredItems observers. | ||
| const { rootCache } = this._dataProviderController; | ||
| rootCache.items = [...rootCache.items]; |
There was a problem hiding this comment.
note: This would become unnecessary if we improved the combo-box's requestContentUpdate to properly handle mutations in filteredItems. The scroller's requestContentUpdate is already capable of handling dropdownItems mutations. However, we also have focused index restoration logic that happens in _setDropdownItems and currently relies on filteredItems being immutable. We could consider improving this separately.
| this.size = undefined; | ||
| this.clearCache(); |
There was a problem hiding this comment.
The requestContentUpdate method is called twice during these two lines. To optimize, we could set the undefined size in the controller instead of the component so that requestContentUpdate is only called in clearCache. What do you think?
There was a problem hiding this comment.
Looks like the tests pass now even with this.size = undefined; removed. Are we missing a test case for this?
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
from
a564ed6
to
e7f0ea6
Compare
wip wip remove hasData API from DataProviderController get rid of page-received fix JSDoc add isPlaceholder for backward compatibility reduce diff fix tests remove unnecessary this binding revert some changes, add more tests wip wip update data provider controller tests minimize diff revert incorrect change after rebase minimize diff remove extra checks improve JSDoc fix corner case, add test enhance JSDoc fix tests reassign cache items to trigger filteredItems observer add code comments fix wip revert check for size from setPage fix failing tests remove unnecessary check revert unnecessary changes use logical assignment operator make data provider controller properties and listeners private
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
from
e7f0ea6
to
b58e6d2
Compare
| size: this.size, | ||
| pageSize: this.pageSize, | ||
| placeholder: new ComboBoxPlaceholder(), | ||
| isPlaceholder: (item) => item instanceof ComboBoxPlaceholder, |
There was a problem hiding this comment.
Seems like this line is untested (all the tests still pass when I remove it).
| * @private | ||
| */ | ||
| this.__dataProviderController = new DataProviderController(this, { | ||
| size: this.size, |
There was a problem hiding this comment.
Seems like this line is untested (all the tests still pass when I remove it).
There was a problem hiding this comment.
Thanks, I removed this line.
| */ | ||
| this.__dataProviderController = new DataProviderController(this, { | ||
| size: this.size, | ||
| pageSize: this.pageSize, |
There was a problem hiding this comment.
Seems like this line is untested (all the tests still pass when I remove it).
Maybe it's not needed as we set page size in the observer?
There was a problem hiding this comment.
Removed this line.
| pageSize: this.pageSize, | ||
| placeholder: new ComboBoxPlaceholder(), | ||
| isPlaceholder: (item) => item instanceof ComboBoxPlaceholder, | ||
| dataProvider: this.dataProvider, |
There was a problem hiding this comment.
Seems like this line is untested (all the tests still pass when I remove it).
Maybe it's not needed as we set dataProvider in the observer?
There was a problem hiding this comment.
Removed this line.
| rootCache.size = size; | ||
| // The controller adds new placeholders to the cache through mutation, | ||
| // so we need to create a new array to trigger filteredItems observers. | ||
| rootCache.items = [...rootCache.items]; |
There was a problem hiding this comment.
Seems like this line is untested (all the tests still pass when I remove it).
| @@ -244,6 +238,8 @@ export const ComboBoxDataProviderMixin = (superClass) => | |||
| this.pageSize = oldPageSize; | |||
| throw new Error('`pageSize` value must be an integer > 0'); | |||
| } | |||
|
|
|||
| this.__dataProviderController.setPageSize(pageSize); | |||
There was a problem hiding this comment.
nit: looks like all the tests pass with clearCache() line below being removed.
But this also happens on main so not related to this PR, can be ignored.
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
3 times, most recently
from
c3695be
to
add3e15
Compare
vursen
force-pushed
the
refactor/combo-box/data-provider-controller
branch
from
add3e15
to
a022592
Compare
|
Description
This PR refactors the combo-box to use DataProviderController. The refactoring is expected to be non-breaking and fully compatible with the existing ComboBox Flow connector, which is verified by successfully passing all the Flow integration tests.
Depends on
Type of change