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

Bk/fix initial attributes target content offset #124

Merged

Conversation

bryankeller
Copy link
Contributor

@bryankeller bryankeller commented Mar 6, 2024

Details

This PR fixes 1 issue (can't find a fix for the other, sadly):

  1. In certain cases, the initial layout attributes of an inserting item would be in the wrong spot. This would happen when the target content offset adjustment was changing the position that the newly inserted item would appear at. We need to compensate for this.

Before:

p1before.mp4

After:

p1after.mp4

  1. [Not fixed, seems like a collection view bug where it skips trying to animate an item since it's end position is off screen] There's an off edge case with collection view that causes a moved item to render instantly at its final position, rather than smoothly sliding over. It's related to self-sizing cells (doesn't happen for statically sized items). All returned layout attributes look correct, so I don't think it's MagazineLayout's fault. It happens when the inserted item / moved item is close to the the top of the screen, which makes me think internally, UICollectionView thinks the moving item isn't visible, and therefore short-circuits its rendering. Interestingly, reducing the estimated item height from 150 -> 10 fixes it. 150 was a magic number, and 10 is too, but I think using a smaller number makes it less likely for collection view to hit this issue.

Before:

p2before.mp4

Related Issue

N/A

Motivation and Context

Bug fixes

How Has This Been Tested

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@bryankeller bryankeller added the bug Something isn't working label Mar 6, 2024
@bryankeller bryankeller force-pushed the bk/fix-initial-attributes-target-content-offset branch 2 times, most recently from 26ed39b to fffd98d Compare March 6, 2024 09:39
@@ -132,7 +132,7 @@ public enum MagazineLayoutItemHeightMode: Hashable {
/// For example, if you support multiline labels or dynamic type, your height is likely not known
/// until the Auto Layout engine resolves the layout at runtime.
public static var dynamic: MagazineLayoutItemHeightMode {
.dynamic(estimatedHeight: MagazineLayout.Default.ItemHeight)
.dynamic(estimatedHeight: 10)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to do some testing to make sure this doesn't break anything in the Airbnb app

Choose a reason for hiding this comment

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

Hmm, not sure about this. I think a lower estimated height could be mean more cells are initially loaded, it's a less accurate estimation. So we could be receiving worse performance to address the animation edge case.

Does the issue only occur if the estimated height puts the original layout attributes frame intersecting the top of the collection view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are good points - I'll do some more investigation

@@ -132,7 +132,7 @@ public enum MagazineLayoutItemHeightMode: Hashable {
/// For example, if you support multiline labels or dynamic type, your height is likely not known
/// until the Auto Layout engine resolves the layout at runtime.
public static var dynamic: MagazineLayoutItemHeightMode {
.dynamic(estimatedHeight: MagazineLayout.Default.ItemHeight)

Choose a reason for hiding this comment

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

See we still use this constant elsewhere, should we change the constant value instead? A bit confused on when we want this smaller height versus not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized we were using this default item height for not just the estimated height, but also the default static height if someone doesn't implement sizeModeForItemAt. Didn't want to cause a regression in that case.

@bryankeller bryankeller force-pushed the bk/fix-initial-attributes-target-content-offset branch 2 times, most recently from c991247 to 99f7e84 Compare March 7, 2024 20:01
@bryankeller bryankeller force-pushed the bk/fix-initial-attributes-target-content-offset branch from 99f7e84 to 2d10f09 Compare March 7, 2024 21:42
@bryankeller bryankeller merged commit 63315fe into master Mar 7, 2024
2 checks passed
@bryankeller bryankeller deleted the bk/fix-initial-attributes-target-content-offset branch March 7, 2024 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants