-
Notifications
You must be signed in to change notification settings - Fork 9
[Feat/#1449] 스프린트 3 API 연결 #1455
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
| viewModelScope.launch { | ||
| _state.value = AppjamtampRankingState.Loading | ||
|
|
||
| val top10Deferred = async { appjamtampRepository.getAppjamtampMissionRanking(size = 12) } |
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 top10Deferred = async { appjamtampRepository.getAppjamtampMissionRanking(size = 12) } | |
| val top10Deferred = async { appjamtampRepository.getAppjamtampMissionRanking(size = 10) } |
(그냥 자나가다) 이거 12로 넣은거 의도한건가요?
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.
이번에 앱잼팀이 총 12팀이라서 12로 넣었어요.
저 api이름이 앱잼팀 오늘의 득점 랭킹 TOP10 조회하기라서 이름을 저렇게 했는데 실제로는 모든 앱잼팀을 다 조회하는 api입니다.
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에 설명 주석 추가해두도록 할게요
Hyobeen-Park
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.
굿도리~~ 꼭 수정해야 할 부분은 크게 없어보여서 어푸푸 눌러둘게요🚀
|
|
||
|
|
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.
진짜 정말 아주 사소한건데.. 빈줄이 2줄이나 들어감!!
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.
앗 이거는 민성님이 만들어둔 것 그대로 가져온거라 (나중에 충돌 안나게 하기 위함 입니다) 그런 것 같아요 나중에 수정하도록 할게요
| createdAt = "2025-12-31T03:20:00", | ||
| userName = "안드로이드", | ||
| userProfileImage = null, | ||
| teamName = "하이링구얼", |
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.
꺆 하이링이닷!!
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(bottom = 56.dp), | ||
| verticalArrangement = Arrangement.spacedBy(space = 10.dp) | ||
| ) { | ||
| rankingList.chunked(size = 2).forEach { rowItems -> | ||
| top10MissionScores.chunked(size = 2).forEach { rowItems -> | ||
| Row( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.spacedBy(space = 10.dp) | ||
| ) { | ||
| rowItems.forEach { item -> | ||
| TodayScoreRaking(modifier = Modifier.weight(weight = 1f)) | ||
| TodayScoreRaking( | ||
| top10MissionScore = item, | ||
| modifier = Modifier.weight(weight = 1f) | ||
| ) |
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과 관련된 내용은 아니긴 한데 그리드 쓰는게 낫지 않으려나요..?? 일단 까먹을까봐 리뷰 남겨두고 갑니다ㅎㅎ
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.
@Hyobeen-Park
지금 이 화면 자체가 전체 스크롤이 가능하게 해두어서 LazyVerticalGrid는 사용이 불가능해요..
(잘 알고 계시겠지만 Lazy 컴포넌트는 스크롤 컨테이너 안에 중첩될 수 없어서요)
대신 FlowRow 사용해서 수정하도록 할게요!
| top10MissionScoreListUiModel = top10Result.getOrThrow().toUiModel() | ||
| ) | ||
| } else { | ||
| _state.value = AppjamtampRankingState.Failure |
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.
@Hyobeen-Park 이거는 에러 다이얼로그를 넣을까 말까 하다가 일단 이렇게 해둔건데 어떻게 하는게 좋다고 생각하시나요?
로그만 넣기에는 그대로 바텀네비에 들어가는 메인화면인데 그렇다고 또 에러 다이얼로그를 넣으면 에러 다이얼로그가 여러 모듈에 각각 퍼져있는 느낌이라..
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.
일단은 로그로만 해둘게요
| import org.sopt.official.domain.appjamtamp.entity.AppjamtampMissionScore | ||
|
|
||
| data class Top10MissionScoreListUiModel( | ||
| val top10MissionScoreList: List<TopMissionScoreUiModel> |
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.
여기에서 ImmutableList로 저장하는게 더 좋지 않을까요?? 데이터 저장/가공하는 부분은 뷰모델까지만 하고 스크린은 소비만 하는게 책임에 맞다는 생각이 들어요! 무엇보다 현재처럼 Screen에서 toImmutableList()를 호출할 경우, 뷰모델/라우트에서는 불변성이 보장되지 않고, recomposition 시마다 새로운 ImmutableList 인스턴스가 생성돼요
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.
이건 제가 원래 쓰는 코드 방식이랑 좀 다르게해서 실수를 좀 했네요
말씀하신대로 UiModel에 바로 immutableList로 하면 굳이 뷰에서 불필요하게 넣을 필요가 없는데 ㅎㅎ
sonms
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.
고생하셨습니다!!! 갓성민
| LaunchedEffect(Unit) { | ||
| viewModel.getRankingData() | ||
| } |
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.
혹시 데이터를 Route에서 호출하신 이유가 있으실까요?(순수 궁금)
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.
Route에서 호출한 이유는 Screen을 순수 UI 코드로만 구성하기 위해서예요.
Screen은 데이터만 받아 렌더링하고, ViewModel 연결/데이터 로딩은 Route에서 처리하면
추후 사이드 이펙트 추가 시에도 Route에서 일괄 관리할 수 있어서 이렇게 구성했어요.
@sonms 혹시 이 구조보다 하나의 Composable(스크린)로 관리하는 게 더 낫다고 생각하시나요?
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.
아닙니다! 제 말에 오해가 있었네요
그 viewmodel의 init 대신 route에서 LaunchedEffect로 호출하신 이유가 궁금해서 그랬습니다 😊
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.
아하 제가 잘못이해했네요 😅
뷰모델 init 안쓰는건 데이터를 갖고오는 시점이 ViewModel 생성시점이라, 새로고침 등 상호작용이나 다른 이벤트에 따라 데이터 로딩할 때 문제가 생길 수도 있어서 init을 선호하지는 않아요(물론 이런 케이스가 발생할 확률은 정말 낮겠지만요 ㅎㅎ)
보통 팀 컨벤션에 맞추는 편인데 여기는 두 방식다 혼용해서 쓰는 것 같아서 이렇게 했어요
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: # data/appjamtamp/src/main/java/org/sopt/official/data/appjamtamp/datasource/AppjamtampDataSource.kt # data/appjamtamp/src/main/java/org/sopt/official/data/appjamtamp/datasourceimpl/AppjamtampDataSourceImpl.kt # data/appjamtamp/src/main/java/org/sopt/official/data/appjamtamp/di/RepositoryModule.kt # data/appjamtamp/src/main/java/org/sopt/official/data/appjamtamp/repository/AppjamtampRepositoryImpl.kt # data/appjamtamp/src/main/java/org/sopt/official/data/appjamtamp/service/AppjamtampService.kt # domain/appjamtamp/src/main/java/org/sopt/official/domain/appjamtamp/repository/AppjamtampRepository.kt
Related issue 🛠
Work Description ✏️
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢
앱잼팀 미션 리스트 뷰 api 연결 작업의 경우 [FEAT] 스프린트 3 - 앱잼탬프를 만들어요 #1443 이 pr에서 구현되어 있어서 해당 내용이 머지 된 이후에 받아서 작업할 예정입니다.
[FEAT] 스프린트 3 - 앱잼탬프를 만들어요 #1443 pr이 머지되면 서버통신 관련 파일, 모듈 파일 충돌해결해야 합니다.
기존에 앱잼 팀 랭킹 컴포넌트를 한개의 파일로 넣어서 만들었는데, 추후 작업할 때 혼동을 유발할 수 있을 것 같아서 각각의 파일로 분리했습니다.