Skip to content

fix: DynamicScroller should pass its own keyField prop to child RecycleScroller #732

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

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

nicooprat
Copy link
Contributor

When using DynamicScroller, the keyField prop was ignored because it was manually set as its default value id here.

…oller

When using DynamicScroller, the `keyField` prop was ignored because it was manually set as its default value `id` here.
@ryanpwaldon
Copy link

Might need to bind the attribute for this one to work 😜

@ryanpwaldon
Copy link

That was fast. Not banking on a merge anytime soon tho 😆🤞

@Akryum Akryum changed the base branch from master to v1 October 10, 2022 17:39
@Akryum Akryum changed the title DynamicScroller should pass its own keyField prop to child RecycleScroller fix: DynamicScroller should pass its own keyField prop to child RecycleScroller Oct 10, 2022
@Akryum Akryum merged commit 9673679 into Akryum:v1 Oct 10, 2022
@Akryum
Copy link
Owner

Akryum commented Oct 18, 2022

I merged this PR too fast, it is expected that DynamicScroller hard code key-field to id.

I will revert this to fix #758

@Akryum
Copy link
Owner

Akryum commented Oct 18, 2022

Items passed from DynamicScroller to RecycleScroller are always of the shape { id, item, size }:

const item = items[i]
const id = simpleArray ? i : item[keyField]
let size = sizes[id]
if (typeof size === 'undefined' && !this.$_undefinedMap[id]) {
size = 0
}
result.push({
item,
id,
size,
})

Akryum added a commit that referenced this pull request Oct 18, 2022
Akryum added a commit that referenced this pull request Oct 18, 2022
@leochen-g
Copy link

This issue has resurfaced in version 2.0, perhaps requiring another round of fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants