-
Notifications
You must be signed in to change notification settings - Fork 0
[Feat] 일정화면 Compose 마이그레이션 #20
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
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
oungsi2000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 제이!
뷰모델을 엎으셨다고 들었는데 고생 많으셨습니다...!
전반적으로 뷰모델이 기존보다 깔끔해진 느낌이 들어 개인적이로 읽기 편했네요 !
하지만 우려하신대로 현재 ScheduleViewModel이 조금 위험하게 동작하고 있어 RC 드립니다..!
고생 많으셨습니다
| style = MaterialTheme.typography.titleLarge, | ||
| color = scheduleEventCardProps.titleColor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이제는 Typhography와 FestabookColor도 CompositionLocal로 전달해주니 MaterialTheme은 안써도 될 것 같아요 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오잉 이건 현재 getter프로퍼티가 없어서 CompositionLocal로 전달 안하고 있는걸로 아는데 제가 놓친 부분이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전역 작업때 추가했습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전역작업pr전에 만든 브랜치라 제 파일에는 반영이 안되어 있는 것 같군요..
리베이스나 머지를 해서 반영할까요 아님 이 pr 머지되면 한번더 작업하는게 좋을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
비교적 간단한 작업이라서 이 PR에 반영해도 좋을 것 같아요 !
| val scheduleEventCardProps = | ||
| when (scheduleEvent.status) { | ||
| ScheduleEventUiStatus.UPCOMING -> { | ||
| ScheduleEventCardProps( | ||
| cardBorderColor = FestabookColor.accentGreen, | ||
| titleColor = FestabookColor.black, | ||
| contentColor = FestabookColor.gray500, | ||
| labelText = stringResource(R.string.schedule_status_upcoming), | ||
| labelTextColor = FestabookColor.black, | ||
| labelBackgroundColor = FestabookColor.white, | ||
| labelBorderColor = FestabookColor.black, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 구현을 보면 ScheduleEventCard이 리컴포지션이 될 때마다, 그리고 각 ScheduleEventCard 컴포저블 별로 독립적인 prop 인스턴스를 생성하게 되어 비효율적인 것 같아요.
사실 데이터들이 정적인 값인데 object로 뺴는건 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드가 사용되는 곳은 ScheduleTabpage.kt -> ScheduleTabContent 밖에 없는데 별도의 파일로 둔 이유가 있으실까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
딱히 대단한 이유는 없지만 파일의 라인이 길어지면 저는 가독성이 떨어진다고 생각해서 분리했습니다.
| scheduleEvent: ScheduleEventUiModel, | ||
| modifier: Modifier = Modifier, | ||
| ) { | ||
| val composition by rememberLottieComposition(LottieCompositionSpec.RawRes(R.raw.pulse_circle)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lottie가 내부적으로 캐싱을 사용하지만, 그래도 매 아이템마다 캐시를 뒤지는 건 여전히 비효율적인것 같아요.
호이스팅하여 lottieComposition을 주입하는건 어떤가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반영했습니다!
refactor: Lottie 프로퍼티 호이스팅 적용
| ScheduleEventUiStatus.COMPLETED -> { | ||
| LottieTimeLineCircleProps( | ||
| centerColor = FestabookColor.gray300, | ||
| outerOpacity = 0f, | ||
| innerOpacity = 0f, | ||
| outerColor = FestabookColor.gray300, | ||
| innerColor = FestabookColor.gray300, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COMPLETED 상태일 때는 lottie를 사용하지 않도록 하는 방향은 어떻게 생각하시나요 ??
한 페이지의 모든 아이템이 Completed이 상태일 때도, Lottie 때문에 hwui에서 막대기가 계속 돌아가네요 ㅠㅠ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
progress를 0f로 변경해서 completed상태일 때 애니메이션을 그리지 않도록 변경했는데 이방법 어떠신가요?
refactor: ScheduleEventItem 코드 구조 개선 및 가독성 향상
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실험해보니 0f로 변경하는 건 단순히 가시성만 변경하는 거라서 실제로 애니메이션 연산은 수행되더라구요..!
animateLottieCompositionAsState로 progress를 받아올 때 상태에 따라 iterations를 손봐야 할 것 같아요.
하지만 해당 방법을 적용하려면 호이스팅하셨던 상태를 제거하고, 기존에 작성하셨던 것처럼 LazyColumn의 각 아이템별로 rememeber ~ 를 해야 할 것 같습니다.
혹은 다른 좋은 방법 있으시다면 적용해도 좋을 것 같아요.
이 부분은 제이의 재량에 맡기겠습니다..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val progress by animateLottieCompositionAsState(
composition = composition,
iterations = LottieConstants.IterateForever,
isPlaying = scheduleEvent.status != ScheduleEventUiStatus.COMPLETED,
)이렇게하고 progress 로그찍어보니까 연산이 멈추네용 이방법 낫배드 인듯?
| supervisorScope { | ||
| scheduleDateUiModels.forEachIndexed { position, scheduleDateUiModel -> | ||
| launch { | ||
| loadEventsByPosition( | ||
| position = position, | ||
| scheduleDateUiModel = scheduleDateUiModel, | ||
| scheduleEventsUiState = scheduleEventUiState, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외 처리를 Result.onFailure로 해주고 있는데 supervisorScope을 사용하신 이유가 있으실까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 날짜탭 하나당 ScheduleUiState를 가지는 방식으로 리팩토링했는데, 만약에 supervisorScope를 사용하지 않으면 fail을 던지고 리턴하게 되버려서 나머지 날짜 탭의 값을 살리기 위해 supervisorScope를 사용했습니다!
혹시 제가 잘못 이해하고 사용한 부분이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supervisorScope는 부모 코루틴으로 예외 전파를 막는 기능인 것으로 알고 있어요.
하지만 지금은 loadEventsByPosition에서 예외가 발생하는 것이 아닌, failure로 예외 처리를 진행해주고 있기 때문에,
굳이 supervisorScope로 감싸지 않아도 동일한 기능을 수행하는 것 같아 드린 리뷰였어요.
fail을 던지고 리턴하게 되버려서
만약 1 번째 루프에서 fail을 한다면 나머지 코루틴들이 중단이 된다는 말씀이실까요?
제가 실험했을 땐 정상 동작했던 것 같습니다 !
| scheduleDateUiModels.forEachIndexed { position, scheduleDateUiModel -> | ||
| launch { | ||
| loadEventsByPosition( | ||
| position = position, | ||
| scheduleDateUiModel = scheduleDateUiModel, | ||
| scheduleEventsUiState = scheduleEventUiState, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 기억이 잘 안나서 그러는데 기존에도 eager loading 방식으로 불러왔나요 ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존에는 ScheduleTabFragment를 생성할 때마다 하나씩 불러오는 방식이었는데 XML의 ViewPager2의 기능 중에 ScheduleTabFragment를 미리 생성해서 캐싱할 수 있었습니다. 이때는 ScheduleTabFragment하나당 ViewModel을 생성하는 구조였기에 eager loading 방식은 아니었습니다.
하지만 HorizontalPager는 fragment를 생성하는게 아닌 컴포저블만 미리 생성해서 그려놓는 방식이기에 일단 eager loading을 사용했는데 이 방식은 비효율적인 것 같아서 loadAllEvents의 파라미터로 pageRange를 추가해서 범위를 지정해서 호출하는 방식으로 변경할 예정입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
흠...빈 화면일 때는 새로고침이 안되네요..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고쳤습니다..😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇..로드에 실패한 탭은 빈화면 + 새로고침이 안되네요...! 이 부분도 제이의 재량에 맡기겠습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 이 부분을 개발할 때 생각을 하긴 했는데 아직 에러처리가 다른화면에서도 미정이라 에러일 때 보여줄 컴포저블을 정하면 그때 반영하려고 남겨두었습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 잘 기억이 안나서 그러는데 기존에도 일정 메뉴 버튼을 재클릭하면 모든 날짜의 일정을 새로고침 했었을까요..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 전체 새로고침으로 기억합니당.
아마 이 기능이 인스타의 UX를 반영했던 걸로 기억하는데 인스타도 메뉴 재클릭하면 게시물과 스토리까지 모두 업데이트하고 초기의 맨 위 게시물로 올라가는걸로 알고있어용
| fun loadSchedules( | ||
| scheduleUiState: ScheduleUiState = ScheduleUiState.InitialLoading, | ||
| scheduleEventUiState: ScheduleEventsUiState = ScheduleEventsUiState.InitialLoading, | ||
| selectedDatePosition: Int? = null, | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
중요 !
현재 의도와는 다르게 특정 페이지를 새로고침하면
해당 페이지의 요소만 불러오는것이 아닌 모든 페이지의 일정 api를 요청하는 문제가 있습니다..!
특정 페이지 새로고침 메소드도 결국 loadAllEvents()을 호출합니다.
다른 부분은 몰라도 이건 꼭 해결을 해주셨으면 좋겠어요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지정된 개수만 불러오도록 변경했습니당.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
잘 동작하네요 !
etama123
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일정화면 PR에 남겨서 죄송합니다 😥
소식 탭에서 리스트 외부 영역을 드래그했을 때, 좌우 페이지로 이동할 수 없는 것 같아요! 한 번 확인부탁드립니다~
고생하셨습니다 제이🐹
| TabRow( | ||
| selectedTabIndex = pageState.currentPage, | ||
| containerColor = MaterialTheme.colorScheme.background, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기에서도 FestabookColor 를 사용할 수 있지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dev브랜치 병합시키고 반영하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기에서도 FestabookColor 를 사용할 수 있지 않을까요?
TabRow가 Material 컴포넌트라서 이렇게 했는데 제가 놓친 부분이 있을까요?
oungsi2000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 제이 ~~
# Conflicts: # app/src/main/java/com/daedan/festabook/di/FestaBookAppGraph.kt # app/src/main/java/com/daedan/festabook/presentation/schedule/ScheduleFragment.kt # app/src/main/java/com/daedan/festabook/presentation/schedule/ScheduleViewModel.kt # app/src/test/java/com/daedan/festabook/schedule/ScheduleViewModelTest.kt
#️⃣ 이슈 번호
#7
🛠️ 작업 내용
ScheduleTabFragment를 삭제하고 ViewModel 객체도 한개만 생성되도록 변경했습니다.
🙇🏻 중점 리뷰 요청
놓친 부분 있으면 알려주세용. 여러 번 엎어서 있을 지도 몰라요..
📸 이미지 첨부 (Optional)
Screen_recording_20251219_214904.mp4