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

[Feat] #39 - Carousel 수정 #97

Merged
merged 8 commits into from
Feb 26, 2023
Merged

[Feat] #39 - Carousel 수정 #97

merged 8 commits into from
Feb 26, 2023

Conversation

JIWON1923
Copy link
Member

@JIWON1923 JIWON1923 commented Feb 23, 2023

Keychanges

  • close [Feat] Carousel 수정 #39
  • Carousel 스크롤 방식을 변경하였습니다.
    • drag를 인식하여 current index를 +1, -1 하던 방식에서 사용자의 drag 크기 및 인덱스를 기준으로 x 좌표 포지션을 조절하였습니다.
    • 기존 로직 상 item이 비어있는지 확인하는 작업을 두 번 확인하고 있었습니다. (snap list, cd list) -> Item의 조회를 1번만 할 수 있도록 로직을 변경하였습니다.
  • UI 변경사항은 다음과 같습니다.
    • 기존 화면에 3개만 보이던 방식 -> 최대 5개가 보입니다.
    • 가운데있는 cd랑 거리가 멀 수록 size 및 opacity가 조정됩니다
    • cd를 클릭하면 해당 인덱스로 이동합니다!

Screenshots

2023-02-23.1.14.40.mov

To Reviewer

  • 카톡방에서 공유드렸던 문제는 index를 벗어나는 문제였습니다. index 1 beyond bounds [0 .. 0]
  • 뷰가 나타날 때, item을 다시 로드하고, current index와 item.count - 1 중 작은 값을 current index로 설정해서 문제를 해결하였습니다.
    • with animation 내부에서 current index를 조절했어서 current index가 변경되기 전, music Information 등에서 이미 삭제된 인덱스를 조회하려고 했던 것 같습니다. 🥲

@JIWON1923 JIWON1923 added the 🌟 enhancement New feature or request label Feb 23, 2023
@JIWON1923 JIWON1923 added this to the version 1.0.3 milestone Feb 23, 2023
@JIWON1923 JIWON1923 self-assigned this Feb 23, 2023
Copy link
Collaborator

@YunSeok-Choi YunSeok-Choi left a comment

Choose a reason for hiding this comment

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

역시 코딩의 신 쏘이!
배워 가요! 👍👍

VStack(spacing: 0) {
musicInformation
cdList
cdPlayer
Copy link
Collaborator

Choose a reason for hiding this comment

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

뷰를 구조체 만들지 않고 변수로 할당하신 이유가 궁금합니다!
구조체로 작성하게되면 유지보수가 훨씬 쉬울 거 같기도 해서요!

Copy link
Member Author

Choose a reason for hiding this comment

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

작업했을 때, 기존에 Item을 뷰마다 fetch하는 방식(cd list view, snap carousel view)에서 벗어나고 싶었어요!
그러다보니 모두 같은 데이터를 공유한다고 생각해서 변수로 할당했었어요

근데 융식 리뷰 보면서 생각해보니 Binding을 사용해서 구조체로 분리할 수 있을 것 같네요!
안 그래도 해당 뷰의 길이가 너무 길어서 고민이었는데 리팩토링 해보겠슴다 ◡̈

private let cdSize: CGFloat = UIScreen.getWidth(200)

private var firstItemPositionX: CGFloat {
(cdSize * CGFloat(max(0, items.count - 1)) + spacing * CGFloat(max(0, items.count - 1))) / 2 + size.width / 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

엄청나네요!👍👍

@JIWON1923
Copy link
Member Author

@YunSeok-Choi 리팩토링 진행했습니다!
저는 구조체 이름이 조금 애매하게 느껴지는데, 여러분들은 어떠신가요! 적절한 네이밍 추천 받습니다.. ^__^

Copy link
Member

@JMM00 JMM00 left a comment

Choose a reason for hiding this comment

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

개발천재 쏘이~~~ 캐러셀을 깔끔하게 구현하셨네요!! 오늘도 배워갑니다!!

뷰 이름들 중 CdListView, CdPlayer만 d가 대문자가 아닌데 혹시 다른 이유가 없다면 대문자나 소문자 둘 중하나로 맞추는건 어떤가요??

@JIWON1923 JIWON1923 merged commit 1ba2693 into develop Feb 26, 2023
@JIWON1923 JIWON1923 deleted the CDListView/Carousel branch February 26, 2023 12:35
@JIWON1923
Copy link
Member Author

개발천재 쏘이~~~ 캐러셀을 깔끔하게 구현하셨네요!! 오늘도 배워갑니다!!

뷰 이름들 중 CdListView, CdPlayer만 d가 대문자가 아닌데 혹시 다른 이유가 없다면 대문자나 소문자 둘 중하나로 맞추는건 어떤가요??

회의 결과에 따라 CD로 모두 통일하여 머지하였습니다~!👍✨

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.

[Feat] Carousel 수정
3 participants