refactor(*): Remove the need of hardcoded item heights in combo #14871
Conversation
mddragnev
added
❌ status: awaiting-test
mddragnev
changed the title
tishko0
added
✅ status: verified
chore(*): fix lint chore(*): fix another lint chore(*): Clean up code. Change some tests and remove some chore(*): Some more cleanup. Fix tests
mddragnev
force-pushed
the
mdragnev/combo-itemheight
branch
from
8d2cfb1 to
7b9d9fc
Compare
| this.dropdown?.animationStarting.subscribe((_args: ToggleViewEventArgs) => { | ||
| // calculate the container size and item size based on the sizes from the DOM | ||
| const dropdownContainerHeight = this.dropdownContainer.nativeElement.getBoundingClientRect().height; | ||
| if (dropdownContainerHeight) { | ||
| this.containerSize = parseFloat(dropdownContainerHeight); | ||
| } | ||
| if (this.dropdown.children?.first) { | ||
| this.itemSize = this.dropdown.children.first.element.nativeElement.getBoundingClientRect().height; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Okay, so this is happening because the overlay opening is emitted before updating sizes, which I realize is likely to check for cancel and avoid doing redundant work. The issue is animationStarting is not guaranteed:
As we discussed, the things updateSize can probably be performed in advance without causing trouble, though I'll tag @wnvko in case he can recall reasons not to.
Alternatively, the combo itself can patch opening and force the update sooner to be able to calculate those on opening.
There was a problem hiding this comment.
Actually, it's also quite possible we could move the --ig-size transfer on attach entirely, since that doesn't change after the fact and it's applied to the content container, if the other stuff are not safe to move.
That's all to avoid both relying on animationStarting and propagating it across components :)
dkamburov
added
❌ status: awaiting-test
tishko0
added
💥 status: in-test
| import { isEqual } from 'lodash-es'; | ||
| import { ToggleViewEventArgs } from '../directives/toggle/toggle.directive'; |
There was a problem hiding this comment.
Pretty sure this is not used anymore
tishko0
added
✅ status: verified
| @@ -189,6 +189,7 @@ | |||
| %igx-combo__content { | |||
| position: relative; | |||
| overflow: hidden; | |||
| max-height: calc(var(--size) * 10); | |||
There was a problem hiding this comment.
This is the baseline assumption of 10 items as the initial chunk size, right? I wonder if we should make this as a CSS variable bound to a property in the business logic, i.e. the initialChunkSize?
There was a problem hiding this comment.
So the thing is that if we set a HostBinding on the combo then we expect to inherit the variable and use it in the igx-combo__content class. However, the overlay service move the dropdown container to a div that is not in the hierarchy of the combo, thus we cannot use that bound variable.
Closes #14285
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)