MBP-4700 Pagination 트리거 인터페이스를 변경 및 개선해요. (BreakingChanges) #44

ppth0608 · 2024-07-02T02:03:43Z

배경

기존 KarrotListKit의 Pagination 처리 방식은 UICollectionViewDelegate의 willDisplayItem:at 함수를 이용하여 마지막 아이템의 index보다 threshold 만큼의 차이가 나는 아이템이 노출되었을때 NextBatchTrigger 이벤트를 콜백하는 방식으로 구현되어 있어요.
위 방식은 Cell의 height에 따라 Trigger 시점이 달라질 수 있기 때문에 의도하지 않는 시점에 다음 페이지를 요청하는 문제가 존재해요.
이를 개선하여, 노출된 아이템의 index 기반이 아닌 ContentOffset 기반으로 리스트의 끝 위치로부터 설정한 위치만큼 도달하게되면 트리거 이벤트를 콜백하는 방식으로 수정하였어요.

변경사항

KarrotListKit 은 다음 페이지 중복 호출에 대한 책임을 갖지 않고, 트리거 조건에 해당하면 무조건 트리거 해주는 심플한 인터페이스로 변경해요.

List(sections: [])
  .onReachedBottom(
    offset: .absolute(100.0),
    perform: { _ in
      // Closure Trigger when reached bottom of list.
    }
  )

onReachedBottom modifier의 트리거 조건은 사용처에서 2가지 방식으로 구성할 수 있어요.
- case multipleHeight(CGFloat): 스크롤 위치가 리스트 뷰 높이의 N배 이내에 있는 경우.
- case absolute(CGFloat): 스크롤 위치가 리스트의 끝에서부터 절대적인 포인트 값 내에 있는 경우.
기본값은 .multipleHeight(2.0)으로 설정되어 있으며, 스크롤 위치가 리스트 뷰 높이의 두 배 이내에 있는 경우 이벤트가 트리거됩니다.

리뷰노트

코드 리뷰는 커밋 단위로 보는 것이 편해요.

OhKanghoon · 2024-07-02T02:05:08Z

오 드디어 올라왔군요

GeekTree0101 · 2024-07-02T05:18:29Z

Sources/KarrotListKit/Event/List/ReachedBottomEvent.swift

+  public struct EventContext {}
+
+  /// Defines the offset from the bottom of the list view that will trigger the event.
+  public enum OffsetFromBottom {


Optional

Vertical 일때만 유효한 네이밍인거 같아요. 네이밍에서 Horizonal scroll 까지 커버할 필요는 없을까요?

행복하세요 벤 🥴❤️

데-하~ 👋 역시 데이빗 좋은 리뷰를 남겨주셨군여 ㅋㅋ
Bottom을 전체적으로 Edge로 변경하면 어떨까요?

사용하는쪽에서 아래와 같이 될 것 같은데 어떻게 느껴지시나요?ㅎㅎ

List {} .onReachedEdge { }

Bottom이 Horizontal에선 맞지 않긴하지만 대부분 UICollectionView의 메인 축을 Verticle을 사용하는 것이 대부분 일반적여서
Bottom suffix가 익숙한 느낌이긴 하네요 😹

onReachedEnd 도 생각나네요 ㅎㅎㅎ scroll direction에 의존해서 vertical은 bottom, horizontal은 right가 될꺼같고 반대 방향은 inversion 고려해라고 독려해볼 수 있을꺼같아요

@jaxtynSong @stleamist @OhKanghoon 인터페이스를 함께 고민해주신 분들 의견은 어떠세요?~

edge보다는 end가 더 익숙하는 느낌이긴하네요 저한테는 ㅎㅎ

.onReachedEnd { }

.onReachedEdge { }

언젠가 inversion을 고려해야한다면 End를 사용하고 Start와 pair를 맞추는 것도 괜찮아보이네요!

edge는 상하좌우 모서리를 포함하는 개념이라, 저도 end에 한 표 던집니다! ㅎㅎ

저도 onReachedEnd 좋아보여요 ㅎㅎ

다들 의견 감사해요 🙏

onReachedEnd로 가볼께요~

4d8cf54에서 반영했어요~

OhKanghoon · 2024-07-02T02:21:22Z

Sources/KarrotListKit/Event/List/ReachedBottomEvent.swift

+  /// Defines the offset from the bottom of the list view that will trigger the event.
+  public enum OffsetFromBottom {
+    /// Triggers the event when the user scrolls within a multiple of the height of the content view.
+    case multipleHeight(CGFloat)


relativeHeight(multiplier: CGFloat) 이나 heightMultiplier(CGFloat) 이 되면 어떨까 생각이 들어요

horizontal까지 고려한다면 case multiple(CGFloat) 도 와드박아봅니다

multiple(CGFloat) 요 네이밍은 고민해봤었는데, 일감으로 이해하기 어렵다고 생각했어요 ㅠ

Verticle 스크롤: ContentSize.Height의 배수

Horizontal 스크롤: ContentSize.Width의 배수

요렇게 될텐데, 조금 길긴하지만 relativeContentSize(multiplier: CGFloat) 요건 어떨까요?~

Cc. @jaxtynSong @stleamist

저는 relativeContentSize 좋아요~

relativeContentSize도 인터페이스 고민시 유사하게 나왔던 네이밍이라 해당 네이밍으로 진행해볼께요!!

contentSize는 스크롤뷰 콘텐트의 전체 크기를 지칭하는 네이밍이기도 해서, 다른 네이밍을 고려해보는 것도 좋을 것 같아요! ㅎㅎ

저는 아래 두 가지 이름을 생각해 봤는데 어떠신가요?

fractional(_:): 동일한 계산식을 사용하는 UICollectionViewCompositionalLayout의 fractionalWidth(_:), fractionalHeight(_:)에 얼라인을 맞춘 네이밍

relativeToContainerSize(multiplier:): 콜렉션뷰의 프레임을 지칭하는 NSCollectionLayoutEnvironment.container를 사용한 네이밍

fractional(_:) 요 네이밍을 고려했었는데, UICollectionViewCompositionalLayout에서는 최댓값이 1.0이더라구요
저희의 케이스의 경우 1.0 이상으로 설정할 수 있어서 자칫 오해가 생기지 않을까 우려가 되었어요..!

레이의 의견과 맥스의 의견을 조합하여 2번 relativeToContainerSize(multiplier:)으로 진행해보겠습니닷

6f1bb2c 에서 반영했어요~

OhKanghoon · 2024-07-02T05:24:46Z

Sources/KarrotListKit/Event/List/ReachedBottomEvent.swift

+  public struct EventContext {}
+
+  /// Defines the offset from the bottom of the list view that will trigger the event.
+  public enum OffsetFromBottom {


Sources/KarrotListKit/Adapter/CollectionViewAdapter.swift

…ier: CGFloat)`으로 리네이밍

README.md

Co-authored-by: Ray (Kanghoon Oh) <[email protected]>

Sources/KarrotListKit/List.swift

README.md

Co-authored-by: Dongkyu Kim (Max.kim) <[email protected]>

Sources/KarrotListKit/Event/List/ReachedBottomEvent.swift

README.md

Sources/KarrotListKit/List.swift

jaxtynSong

고생 많으셨어요!!!

OhKanghoon · 2024-07-02T09:21:35Z

Sources/KarrotListKit/Adapter/CollectionViewAdapter.swift

+extension CollectionViewAdapter {
+
+  private var scrollDirection: UICollectionView.ScrollDirection {
+    let layout = collectionView?.collectionViewLayout as? UICollectionViewCompositionalLayout


TODO: 특정 레이아웃에 종속되지 않게 변경할 때 챙기기

요거 코멘트 추가해둘까요?~

Issue 만들어서 기록하려고 해서
코멘트는 따로 안달아주셔도 괜찮아요!

OhKanghoon

고생하셨어요 👍 👍 👍

stleamist

고생하셨어요! 👍🏻

ppth0608 · 2024-07-02T09:33:24Z

리뷰해주신 @OhKanghoon @jaxtynSong @stleamist @GeekTree0101 감사합니다 🙏

GeekTree0101

LGTM

Ben.park added 4 commits July 2, 2024 09:25

feat: 기존 NextBatchTrigger 관련 코드 및 문서 제거

d3dd95b

feat: ReachedBottomEvent 추가 및 modifier를 List에 추가

fd61b30

feat: ReachedBottomEvent Triggering 로직 추가

39c2f00

feat: README.md에 사용법 추가

2cfef3e

ppth0608 added Improvement Update labels Jul 2, 2024

ppth0608 self-assigned this Jul 2, 2024

ppth0608 requested review from OhKanghoon and jaxtynSong as code owners July 2, 2024 02:03

OhKanghoon added Breaking Changes and removed Update labels Jul 2, 2024

GeekTree0101 reviewed Jul 2, 2024

View reviewed changes

OhKanghoon reviewed Jul 2, 2024

View reviewed changes

review: Bottm 대신 End 네이밍 사용

4d8cf54

stleamist reviewed Jul 2, 2024

View reviewed changes

Sources/KarrotListKit/Adapter/CollectionViewAdapter.swift Outdated Show resolved Hide resolved

reivew: .multipleHeight(CGFloat)를 `.relativeToContainerSize(multipl…

c51243c

…ier: CGFloat)`으로 리네이밍

ppth0608 force-pushed the feature/ben/MBP-4700-next-batch-trigger-2 branch from 6f1bb2c to c51243c Compare July 2, 2024 07:38

OhKanghoon reviewed Jul 2, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update README.md

1739ac7

Co-authored-by: Ray (Kanghoon Oh) <[email protected]>

jaxtynSong reviewed Jul 2, 2024

View reviewed changes

Sources/KarrotListKit/List.swift Outdated Show resolved Hide resolved

stleamist reviewed Jul 2, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update README.md

5c725b0

Co-authored-by: Dongkyu Kim (Max.kim) <[email protected]>

stleamist reviewed Jul 2, 2024

View reviewed changes

Sources/KarrotListKit/Event/List/ReachedBottomEvent.swift Outdated Show resolved Hide resolved

jaxtynSong reviewed Jul 2, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

stleamist reviewed Jul 2, 2024

View reviewed changes

Sources/KarrotListKit/List.swift Outdated Show resolved Hide resolved

Ben.park added 3 commits July 2, 2024 17:35

review: perform 파라미터를 handler로 네이밍 변경

dfd07fb

review: Bottom 네이밍이 사용중인 곳 일관 수정

f9112f9

review: onReachedBottom 함수를 onReachBottom으로 리네이밍

1bea6e0

jaxtynSong approved these changes Jul 2, 2024

View reviewed changes

OhKanghoon reviewed Jul 2, 2024

View reviewed changes

OhKanghoon approved these changes Jul 2, 2024

View reviewed changes

stleamist approved these changes Jul 2, 2024

View reviewed changes

GeekTree0101 approved these changes Jul 2, 2024

View reviewed changes

ppth0608 merged commit 0e80b84 into main Jul 2, 2024
1 check passed

ppth0608 deleted the feature/ben/MBP-4700-next-batch-trigger-2 branch July 2, 2024 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBP-4700 Pagination 트리거 인터페이스를 변경 및 개선해요. (BreakingChanges) #44

MBP-4700 Pagination 트리거 인터페이스를 변경 및 개선해요. (BreakingChanges) #44

ppth0608 commented Jul 2, 2024

OhKanghoon commented Jul 2, 2024

GeekTree0101 Jul 2, 2024 •

edited

Loading

GeekTree0101 Jul 2, 2024

OhKanghoon Jul 2, 2024

ppth0608 Jul 2, 2024

GeekTree0101 Jul 2, 2024

ppth0608 Jul 2, 2024

stleamist Jul 2, 2024

jaxtynSong Jul 2, 2024

ppth0608 Jul 2, 2024

ppth0608 Jul 2, 2024

OhKanghoon Jul 2, 2024

GeekTree0101 Jul 2, 2024

ppth0608 Jul 2, 2024

OhKanghoon Jul 2, 2024 •

edited

Loading

ppth0608 Jul 2, 2024

stleamist Jul 2, 2024 •

edited

Loading

ppth0608 Jul 2, 2024

ppth0608 Jul 2, 2024

OhKanghoon Jul 2, 2024

ppth0608 Jul 2, 2024

OhKanghoon Jul 2, 2024

jaxtynSong left a comment

OhKanghoon Jul 2, 2024

ppth0608 Jul 2, 2024

OhKanghoon Jul 2, 2024

OhKanghoon left a comment

stleamist left a comment

ppth0608 commented Jul 2, 2024

GeekTree0101 left a comment

MBP-4700 Pagination 트리거 인터페이스를 변경 및 개선해요. (BreakingChanges) #44

MBP-4700 Pagination 트리거 인터페이스를 변경 및 개선해요. (BreakingChanges) #44

Conversation

ppth0608 commented Jul 2, 2024

배경

변경사항

리뷰노트

OhKanghoon commented Jul 2, 2024

GeekTree0101 Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Optional

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OhKanghoon Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stleamist Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaxtynSong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OhKanghoon left a comment

Choose a reason for hiding this comment

stleamist left a comment

Choose a reason for hiding this comment

ppth0608 commented Jul 2, 2024

GeekTree0101 left a comment

Choose a reason for hiding this comment

GeekTree0101 Jul 2, 2024 •

edited

Loading

OhKanghoon Jul 2, 2024 •

edited

Loading

stleamist Jul 2, 2024 •

edited

Loading