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.
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
[labs/virtualizer] RangeChangedEvent and VisibilityChangedEvent no longer bubble up #3609
Conversation
🦋 Changeset detectedLatest commit: 0ddcf55 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultslit-element-list
render
update
update-reflect
lit-html-kitchen-sink
render
update
nop-update
lit-html-repeat
render
update
lit-html-template-heavy
render
update
reactive-element-list
render
update
update-reflect
|
0de6f79
to
1ae6a28
Compare
1ae6a28
to
0ddcf55
Compare
CC: @Westbrook @najikahalsema @graynorton this PR wraps up concerns discussed in issue #3051 about Virtualizer's event propagation |
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.
LGTM!
The comments are just some musings from not being familiar with the code. This can go in as-is.
@@ -11,7 +11,7 @@ export class RangeChangedEvent extends Event { | |||
last: number; | |||
|
|||
constructor(range: Range) { | |||
super(RangeChangedEvent.eventName, {bubbles: true}); | |||
super(RangeChangedEvent.eventName, {bubbles: false}); |
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 first
and last
to 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
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type
is only used for attribute converters (it would be more accurately named typeHint
), so if a property has attribute: false
it doesn't need a type at all. For test elements, it's academic though and you could just leave all properties off. The converter would only kick in if reflecting to an attribute (which is off) or setting from an attribute, which you're not doing.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my edification, do you know why we need to wait for these elements first with the until()
call above? The elements are defined in the module, and before the fixture
call, which itself is awaited. I wouldn't think the until()
s are needed at all. but if there was a need to wait for an upgrade, customElements.whenDefined('using-virtualize-directive')
should work too.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
expect(uvd.shadowRoot?.textContent).to.include('2 selected'); | ||
expect(uvd.shadowRoot?.textContent).to.include('5 selected'); | ||
expect(ulv.shadowRoot?.textContent).not.to.include('[2 selected]'); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair points. I'll add this when I do some overall test sprucing up in #2788
This PR solves the event behavior differences between RangeChangedEvent and VisibilityChangedEvent per #3051
Both event types are ensured not to bubble up and must be listened for now on the
<lit-virtualizer>
itself or the host element in the case ofvirtualize()
directive.