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 Incorrect Supplementary Item Found During WillDisplay and DidEndDisplaying Callbacks #147

Conversation

brynbodayle
Copy link
Contributor

@brynbodayle brynbodayle commented Aug 15, 2023

Change summary

How was it tested?

How did you verify that this change accomplished what you expected? Add more detail as needed.

  • Wrote automated tests
  • Built and ran on the iOS simulator
  • Built and ran on a device

Pull request checklist

All items in this checklist must be completed before a pull request will be reviewed.

  • Risky changes have been put behind a feature flag, e.g. CollectionViewConfiguration
  • Added a CHANGELOG.md entry in the "Unreleased" section for any library changes

@brynbodayle brynbodayle force-pushed the bb-supplementary-view-display-callbacks-during-updates-fix branch from 5ca6ebe to 21ea71c Compare August 15, 2023 18:55
@brynbodayle brynbodayle force-pushed the bb-supplementary-view-display-callbacks-during-updates-fix branch from d5a8006 to b47778b Compare August 15, 2023 22:16
guard let view = view as? CollectionViewReusableView else {
// We don't assert since `UICollectionViewCompositionalLayout` can create and configure its
// own supplementary views e.g. with a `.list(using: .init(appearance: .plain))` config.
return
Copy link
Contributor

Choose a reason for hiding this comment

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

oof, this sucks. I know we're currently using the cell / reusable view as an easy way to store the item path, but maybe that isn't the right solution (given this limitation). TBH I'm cool with ignoring this for now, but we might need to address this in the future by maintaining a cell / reusable view => item path mapping ourselves.

Copy link
Contributor Author

@brynbodayle brynbodayle Aug 15, 2023

Choose a reason for hiding this comment

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

What is the limitation you're concerned about? We only need to provide callbacks and track the visibility of our own Epoxy-based views, not ones created internally by compositional layout.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you're right. I was thinking we'd still want callbacks for those, but makes sense that we wouldn't since we're not making the epoxy models for them.

…ctionViewReusableView.swift

Co-authored-by: Bryan Keller <kellerbryan19@gmail.com>
@brynbodayle brynbodayle merged commit 4042b90 into airbnb:master Aug 16, 2023
9 checks passed
@brynbodayle brynbodayle deleted the bb-supplementary-view-display-callbacks-during-updates-fix branch August 16, 2023 01:00
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.

None yet

2 participants