Skip to content
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

Fix element lookup for supplementary attributes. #1707

Conversation

craighowarth
Copy link
Contributor

The _measureElementsInRect() function in ASCollectionLayout.mm was performing a [elementForItemAtIndexPath:] on the index path for every attribute. Some of those attributes might be supplementary views, but this distinction was being ignored. The fix will check the representedElementKind to see if the item is a supplementary view and if it is, [supplementaryElementOfKind:atIndexPath:] will be called instead.

@craighowarth
Copy link
Contributor Author

I have accepted the terms for the license. Not sure why the PR hasn't approved that last step. Let me know if there is a step that I missed.

@maicki
Copy link
Contributor

maicki commented Oct 22, 2019

@craighowarth Thanks for this PR and sorry that it took a couple of days. LGTM but I would also like to have eyes from @Adlai-Holler and @nguyenhuy on this.

Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too. Thanks for the fix!

@nguyenhuy nguyenhuy merged commit b59c6d0 into TextureGroup:master Dec 24, 2019
@appleguy
Copy link
Member

Seeing this 6 months later, but wanted to thank you @craighowarth for your analysis of the code and the resulting contribution. It's sincerely appreciated!

Supplementary items were not widely utilized by the teams that depended on Texture the most as this code was built out, but it's really valuable for the community to isolate any of the faults in those codepaths.

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.

5 participants