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

Allow input estimate height for height mode dynamic #113

Merged
merged 2 commits into from
Apr 1, 2023

Conversation

elliot-vu
Copy link
Contributor

@elliot-vu elliot-vu commented Mar 30, 2023

Details

Add estimatedHeight argument to MagazineLayoutItemSizeMode dynamic

Related Issue

#56

Motivation and Context

Resolve the collection view jerking after reload data

How Has This Been Tested

Implement the collection view contains multi item with dynamic height mode. Scroll collection view to bottom and reload data

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.

@elliot-vu
Copy link
Contributor Author

@brynbodayle Please help me review this PR

@bryankeller
Copy link
Contributor

bryankeller commented Mar 31, 2023

Thanks for the change! I think this makes sense at a high level.

What if instead of adding a new delegate method, we change the MagazineLayoutItemHeightMode enum like this:

/// Represents the vertical sizing mode for an item.
///
/// `MagazineLayout` supports vertically self-sizing and statically sized items. Since height modes
/// are specified for each item, you can mix vertically self-sizing and statically sized items in
/// the same sections, and even in the same rows.
public enum MagazineLayoutItemHeightMode: Hashable {

  /// This height mode mode will cause the item to be displayed with a height equal to `height`.
  ///
  /// To properly support multiline labels, dynamic type, and other technologies that could affect
  /// the height of your items dynamically, consider using one of the dynamic height modes.
  case `static`(height: CGFloat)

  /// This height mode will cause the item to self-size in the vertical direction, using the `estimatedHeight` as an initial
  /// placeholder.
  ///
  /// In practice, self-sizing in the vertical direction means that the item will get its height
  /// from the Auto Layout engine. Use this height mode for items whose height is not known upfront.
  /// 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.
  case dynamic(estimatedHeight: CGFloat)

  /// This height mode will cause the item to self-size in the vertical direction, then resize to
  /// match the height of the tallest item in the same row of items.
  ///
  /// If the item _is_ the tallest item in the row (after being self-sized), then it will stay
  /// at its self-sized height until it's no longer the tallest item in the row.
  ///
  /// Note that items with this height mode will resize to match the height of the tallest item in
  /// the same row of items, even if the tallest item has a `static` height mode.
  case dynamicAndStretchToTallestItemInRow

  /// This height mode will cause the item to self-size in the vertical direction
  ///
  /// In practice, self-sizing in the vertical direction means that the item will get its height
  /// from the Auto Layout engine. Use this height mode for items whose height is not known upfront.
  /// 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)
  }

}

@elliot-vu
Copy link
Contributor Author

Thanks for the change! I think this makes sense at a high level.

What if instead of adding a new delegate method, we change the MagazineLayoutItemHeightMode enum like this:

/// Represents the vertical sizing mode for an item.
///
/// `MagazineLayout` supports vertically self-sizing and statically sized items. Since height modes
/// are specified for each item, you can mix vertically self-sizing and statically sized items in
/// the same sections, and even in the same rows.
public enum MagazineLayoutItemHeightMode: Hashable {

  /// This height mode mode will cause the item to be displayed with a height equal to `height`.
  ///
  /// To properly support multiline labels, dynamic type, and other technologies that could affect
  /// the height of your items dynamically, consider using one of the dynamic height modes.
  case `static`(height: CGFloat)

  /// This height mode will cause the item to self-size in the vertical direction, using the `estimatedHeight` as an initial
  /// placeholder.
  ///
  /// In practice, self-sizing in the vertical direction means that the item will get its height
  /// from the Auto Layout engine. Use this height mode for items whose height is not known upfront.
  /// 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.
  case dynamic(estimatedHeight: CGFloat)

  /// This height mode will cause the item to self-size in the vertical direction, then resize to
  /// match the height of the tallest item in the same row of items.
  ///
  /// If the item _is_ the tallest item in the row (after being self-sized), then it will stay
  /// at its self-sized height until it's no longer the tallest item in the row.
  ///
  /// Note that items with this height mode will resize to match the height of the tallest item in
  /// the same row of items, even if the tallest item has a `static` height mode.
  case dynamicAndStretchToTallestItemInRow

  /// This height mode will cause the item to self-size in the vertical direction
  ///
  /// In practice, self-sizing in the vertical direction means that the item will get its height
  /// from the Auto Layout engine. Use this height mode for items whose height is not known upfront.
  /// 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)
  }

}

Agree. It has been updated. Please assist me in going over it again
@bryankeller

@bryankeller bryankeller self-requested a review April 1, 2023 00:52
@bryankeller bryankeller added the enhancement New feature or request label Apr 1, 2023
@bryankeller bryankeller self-assigned this Apr 1, 2023
Copy link
Contributor

@bryankeller bryankeller left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@elliot-vu
Copy link
Contributor Author

Great, thanks!

Your CI Pipeline has error:

xcodebuild: error: Unable to find a destination matching the provided destination specifier:
		{ OS:15.2, name:iPhone 8 }

Could you fix it

@bryankeller
Copy link
Contributor

Merging past failing CI, will fix CI separately. Thanks for your patience @elliot-vu ! And for making this useful change.

@bryankeller bryankeller merged commit 2503c40 into airbnb:master Apr 1, 2023
0 of 2 checks passed
@bryankeller
Copy link
Contributor

@elliot-vu you can point your Podfile or SPM to the latest commit, rather than the latest tagged version, so that you're not blocked by me.

@alexookah
Copy link

@bryankeller Is there any plans to make a new tag version that includes these changes?

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

Successfully merging this pull request may close these issues.

None yet

3 participants