[Feat] 일정화면 Compose 마이그레이션 #22
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note
|
There was a problem hiding this comment.
고생하셨습니다 타마
궁금한 점 코멘트 남겨봤습니다 확인 부탁드려요.
그리고 PR 제목 수정 부탁드려요..ㅋㅋ
홈 화면 로딩 관련
모든 상태(festival, lineup 등등)이 성공 상태일 때, 홈 화면 전체를 보여주는 게 좋을 까요? 아니면 각 항목별로 로딩 상태일 때의 액션(스켈레톤 등)을 추가하는 게 좋을까요?FestabookColor.white
해당 속성이 기존에 사용하던 배경색(진짜 쌩 화이트)가 아니라 새롭게 지정한 디자인 시스템의 white 입니다. 따라서 화면의 배경색을 지정할 때,
FestabookColor.white를 사용하지 못하고 Color.white를 사용해야합니다. 이 점에 대해 따로 배경색을 추가하는 방향은 어떤 지 논의하고 싶습니다.
- 분리하는게 좋을 것 같아요
- Color.white를 사용해야만 하는 이유가 뭔가요?
Scaffold는 색들이 자동으로 materialTheme을 따라가서 저희 FestabookTheme 색 정의에 대한 논의를 할 때 얘기해보면 좋을 것 같아용.
| import timber.log.Timber | ||
|
|
||
| @ContributesIntoMap(scope = AppScope::class, binding = binding<Fragment>()) | ||
| @ContributesIntoMap(scope = AppScope::class, binding = binding<androidx.fragment.app.Fragment>()) |
There was a problem hiding this comment.
임포트를 안하고 사용하신 이유가 있으실까요?
| class HomeViewModel @Inject constructor( | ||
| private val festivalRepository: FestivalRepository, |
There was a problem hiding this comment.
클래스에 @Inject를 붙이는걸로 변경하는게 좋을 것 같아용
| homeViewModel.navigateToScheduleEvent.collectLatest { | ||
| binding.bnvMenu.selectedItemId = R.id.item_menu_schedule | ||
| } | ||
| } |
There was a problem hiding this comment.
collectLatest 를 사용하신 이유가 궁금합니다.
| Column( | ||
| modifier = modifier.fillMaxWidth(), | ||
| ) { | ||
| Text( | ||
| text = festivalName, | ||
| style = FestabookTypography.displayMedium, | ||
| color = FestabookColor.black, | ||
| modifier = Modifier.padding(horizontal = 20.dp), | ||
| ) | ||
|
|
||
| Spacer(modifier = Modifier.height(8.dp)) | ||
|
|
||
| Text( | ||
| text = festivalDate, | ||
| style = FestabookTypography.bodyLarge, | ||
| color = FestabookColor.gray500, | ||
| modifier = Modifier.padding(horizontal = 20.dp), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Text에서 padding을 주지 않고 Column에서 한 번에 주는건 어떄요?
| Image( | ||
| painter = painterResource(id = R.drawable.ic_dropdown), | ||
| contentDescription = stringResource(id = R.string.home_navigate_to_explore_desc), | ||
| ) |
There was a problem hiding this comment.
Icon함수가 아닌 Image함수를 사용하신 이유가 궁금해요
| Scaffold( | ||
| modifier = modifier.fillMaxSize(), | ||
| containerColor = Color.White, | ||
| ) { |
There was a problem hiding this comment.
슬렉에서 말씀하신 빨간줄이 여기군용.
Scaffold를 사용하고 있는데 content에서 PaddingValues를 사용하지 않고 있어서 빨간줄 뜨는걸로 알고 있습니다.
| modifier = | ||
| Modifier | ||
| .width(itemWidth) | ||
| .height(400.dp) | ||
| .graphicsLayer { | ||
| scaleX = scale | ||
| scaleY = scale | ||
| this.alpha = alpha | ||
| } | ||
| .clip(RoundedCornerShape(10.dp)), |
There was a problem hiding this comment.
공통컴포저블인 cardBackground를 사용하는 것도 좋아보이네용
| item { | ||
| Box(modifier = Modifier.padding(bottom = 60.dp)) | ||
| } |
There was a problem hiding this comment.
다른건 전부 Spacer였는데 여기는Box로 하신 이유가 있으실까요?
| // 라인업 리스트 | ||
| when (lineupUiState) { | ||
| is LineupUiState.Success -> { | ||
| val lineups = lineupUiState.lineups.getLineupItems() |
There was a problem hiding this comment.
여기 프로퍼티가 아니라 getLineupItems함수로 해야했던 이유가 있었을까요?(진짜 모름)
| Box( | ||
| modifier = | ||
| Modifier | ||
| .padding(horizontal = 16.dp) | ||
| .width(75.dp) | ||
| .height(1.dp) | ||
| .background(FestabookColor.gray700), | ||
| ) |
There was a problem hiding this comment.
HorizontalDivider사용하는건 어떠신가요?
프리뷰를 봤을 때 오늘이면 divider와 날짜+오늘 텍스트의 width가 미세하게 다릅니다!
divider와 텍스트를 하나의 컴포저블로 묶어서 사이즈 관리를 하면 수월하지 않을까 합니다!
#️⃣ 이슈 번호
🛠️ 작업 내용
🙇🏻 중점 리뷰 요청
주의할 부분
논의할 부분
1. 홈 화면 로딩 관련
모든 상태(festival, lineup 등등)이 성공 상태일 때, 홈 화면 전체를 보여주는 게 좋을 까요? 아니면 각 항목별로 로딩 상태일 때의 액션(스켈레톤 등)을 추가하는 게 좋을까요?
2. FestabookColor.white
해당 속성이 기존에 사용하던 배경색(진짜 쌩 화이트)가 아니라 새롭게 지정한 디자인 시스템의 white 입니다. 따라서 화면의 배경색을 지정할 때,
FestabookColor.white를 사용하지 못하고 Color.white를 사용해야합니다. 이 점에 대해 따로 배경색을 추가하는 방향은 어떤 지 논의하고 싶습니다.
📸 이미지 첨부 (Optional)