-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
feat(cdk/scrolling): update CdkVirtualForOf to work with sets. #20594
Conversation
72b2761
to
3a0f4fa
Compare
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.
Can you add a unit test to make sure it works? Also the way the commit message is formatted won't auto-close the related issue. It should say Fixes #<issue number>
or Resolves #<issue number>
.
The previous usage of `Array.prototype.slice.call` does not handle `Set` objects appropriately (since a `Set` does not have a `length` property). Updated it to use `Array.from`. Fixes angular#20210.
3a0f4fa
to
d6fbd46
Compare
Thanks for the review! I updated the commit message. I believe the current test make sure that the feature works -- is there anything additional you would like added? There's no corresponding virtual-for-of.spec.ts file, so the existing test seemed like the most appropriate one for me to add. |
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.
The test you added should be enough. LGTM.
@crisbeto What's going on with CI here? |
Not sure, but it wasn't triggered for some reason. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
The previous usage of
Array.prototype.slice.call
does not handleSet
objects appropriately (since a
Set
does not have alength
property).Updated it to use
Array.from
.Fixes #20210.