[labs/virtualizer] RangeChangedEvent and VisibilityChangedEvent no longer bubble up #3609
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@lit-labs/virtualizer': minor | ||
| --- | ||
|
|
||
| RangeChangedEvent and VisibilityChangedEvent both no longer bubble up. Listeners for these events must be placed on the lit-virtualizer or virtualize directive's host element. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,15 +7,17 @@ | |
| import {array, ignoreBenignErrors, until} from '../helpers.js'; | ||
| import {LitVirtualizer} from '../../lit-virtualizer.js'; | ||
| import {virtualize} from '../../virtualize.js'; | ||
| import {RangeChangedEvent, VisibilityChangedEvent} from '../../events.js'; | ||
| import {css, LitElement} from 'lit'; | ||
| import {customElement, property} from 'lit/decorators.js'; | ||
| import {expect, html, fixture} from '@open-wc/testing'; | ||
|
|
||
| abstract class TestElement extends LitElement { | ||
| static styles = css` | ||
| .item { | ||
| height: 50px; | ||
| margin: 0; | ||
| padding: 0; | ||
| } | ||
| `; | ||
|
|
||
|
|
@@ -24,29 +26,52 @@ abstract class TestElement extends LitElement { | |
|
|
||
| @property({type: Array, attribute: false}) | ||
| public items: Array<number> = []; | ||
|
|
||
| @property({type: Array, attribute: false}) | ||
|
There was a problem hiding this comment.
|
||
| public rangeChangedEvents: RangeChangedEvent[] = []; | ||
|
|
||
| @property({type: Array, attribute: false}) | ||
| public visibilityChangedEvents: VisibilityChangedEvent[] = []; | ||
|
|
||
| recordRangeChangedEvent(event: RangeChangedEvent) { | ||
| this.rangeChangedEvents.push(event); | ||
| } | ||
|
|
||
| recordVisibilityChangedEvent(event: VisibilityChangedEvent) { | ||
| this.visibilityChangedEvents.push(event); | ||
| } | ||
| } | ||
|
|
||
| @customElement('using-lit-virtualizer') | ||
| class UsingLitVirtualizer extends TestElement { | ||
| render() { | ||
| return html` <lit-virtualizer | ||
| @rangeChanged=${this.recordRangeChangedEvent} | ||
| @visibilityChanged=${this.recordVisibilityChangedEvent} | ||
| scroller | ||
| .items=${this.items} | ||
| .renderItem=${(n: number) => | ||
| html`<div class="item"> | ||
| [${n}${this.selected.has(n) ? ' selected' : ''}] | ||
| </div>`} | ||
| ></lit-virtualizer>`; | ||
| } | ||
| } | ||
|
|
||
| @customElement('using-virtualize-directive') | ||
| class UsingVirtualizeDirective extends TestElement { | ||
| render() { | ||
| return html` <div | ||
| @rangeChanged=${this.recordRangeChangedEvent} | ||
| @visibilityChanged=${this.recordVisibilityChangedEvent} | ||
| > | ||
| ${virtualize({ | ||
| scroller: true, | ||
| items: this.items, | ||
| renderItem: (n) => | ||
| html`<div class="item"> | ||
| [${n}${this.selected.has(n) ? ' selected' : ''}] | ||
| </div>`, | ||
| })} | ||
| </div>`; | ||
| } | ||
|
|
@@ -81,6 +106,14 @@ describe('lit-virtualizer and virtualize directive', () => { | |
| <using-lit-virtualizer></using-lit-virtualizer> | ||
| <using-virtualize-directive></using-virtualize-directive> | ||
| </div> | ||
| <style> | ||
| using-lit-virtualizer { | ||
| height: 500px; | ||
| } | ||
| using-virtualize-directive { | ||
| height: 500px; | ||
| } | ||
| </style> | ||
| `); | ||
| await until( | ||
| () => | ||
|
|
@@ -96,45 +129,67 @@ describe('lit-virtualizer and virtualize directive', () => { | |
| const ulv: UsingLitVirtualizer = example.querySelector( | ||
| 'using-lit-virtualizer' | ||
| )!; | ||
| const uvd: UsingVirtualizeDirective = example.querySelector( | ||
| 'using-virtualize-directive' | ||
|
There was a problem hiding this comment. For my edification, do you know why we need to wait for these elements first with the I'm asking because the test is so long that any reduction in boilerplate will help new people read it. There was a problem hiding this comment. Yeah this was how I was watching for upgrades from the outside. I didn't want to select the element until after the upgrade occurred, but I'll see if I can either eliminate the need for those, write a helper, or comment why it's necessary to explicitly await the upgrades for the elements when I do test clean ups in #2788 |
||
| )!; | ||
|
|
||
| ulv.items = [...items]; | ||
| uvd.items = [...items]; | ||
|
|
||
| ulv.selected = selected; | ||
| uvd.selected = selected; | ||
|
|
||
| await until(() => ulv.shadowRoot?.textContent?.includes('[5 selected]')); | ||
| await until(() => uvd.shadowRoot?.textContent?.includes('[5 selected]')); | ||
|
|
||
| expect(ulv.shadowRoot?.textContent).to.include('[2 selected]'); | ||
| expect(uvd.shadowRoot?.textContent).to.include('[2 selected]'); | ||
|
|
||
| expect(ulv.shadowRoot?.textContent).to.include('[5 selected]'); | ||
| expect(uvd.shadowRoot?.textContent).to.include('[5 selected]'); | ||
|
|
||
| // Changing selection doesn't trigger visibility changed or range changed events. | ||
| ulv.selected = new Set([1, 3]); | ||
| uvd.selected = new Set([1, 3]); | ||
|
|
||
| await until(() => ulv.shadowRoot?.textContent?.includes('[3 selected]')); | ||
| await until(() => uvd.shadowRoot?.textContent?.includes('[3 selected]')); | ||
|
|
||
| expect(ulv.shadowRoot?.textContent).to.include('[1 selected]'); | ||
| expect(uvd.shadowRoot?.textContent).to.include('[1 selected]'); | ||
|
|
||
| expect(ulv.shadowRoot?.textContent).to.include('[3 selected]'); | ||
| expect(uvd.shadowRoot?.textContent).to.include('[3 selected]'); | ||
|
|
||
| expect(ulv.shadowRoot?.textContent).not.to.include('[2 selected]'); | ||
|
There was a problem hiding this comment. Maybe not now, but since the test is so long, it'd be great to see some comments break it up and describe the scenario, what the setup is doing, what the interaction is, expectations are checking, etc. There was a problem hiding this comment. Fair points. I'll add this when I do some overall test sprucing up in #2788 |
||
| expect(uvd.shadowRoot?.textContent).not.to.include('[2 selected]'); | ||
|
|
||
| expect(ulv.shadowRoot?.textContent).not.to.include('[5 selected]'); | ||
| expect(uvd.shadowRoot?.textContent).not.to.include('[5 selected]'); | ||
|
|
||
| // Clearing event arrays so we can watch for specific future events. | ||
| ulv.rangeChangedEvents.splice(0); | ||
| uvd.rangeChangedEvents.splice(0); | ||
| ulv.visibilityChangedEvents.splice(0); | ||
| uvd.visibilityChangedEvents.splice(0); | ||
|
|
||
| // Adding an item to the start of the list to trigger rangechanged and | ||
| // visibilitychanged events. | ||
| ulv.items = [-1, ...items]; | ||
| uvd.items = [-1, ...items]; | ||
|
|
||
| await until(() => ulv.shadowRoot?.textContent?.includes('[-1]')); | ||
| await until(() => uvd.shadowRoot?.textContent?.includes('[-1]')); | ||
|
|
||
| await until(() => ulv.rangeChangedEvents.length > 0); | ||
| await until(() => uvd.rangeChangedEvents.length > 0); | ||
|
|
||
| expect(ulv.rangeChangedEvents.length).to.equal(1); | ||
| expect(uvd.rangeChangedEvents.length).to.equal(1); | ||
|
|
||
| // The indexes of visible items have not changed even though new item was | ||
| // added to head of the array. So no visibilitychanged events are expected. | ||
| expect(ulv.visibilityChangedEvents.length).to.equal(0); | ||
| expect(uvd.visibilityChangedEvents.length).to.equal(0); | ||
| }); | ||
| }); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but can we get code comments on the event class and on
firstandlastto document to users and maintainers what they're for? It can just be what's in the readme here: https://github.com/lit/lit/tree/main/packages/labs/virtualizer#visibilitychanged-event