[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
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | PR 제목은 주요 변경 사항인 홈화면의 Compose 마이그레이션을 명확하게 설명하며, 개발자 관점에서 핵심 변경사항을 정확히 요약하고 있습니다. |
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 @coderabbitai help to get the list of available commands and usage tips.
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 를 사용하신 이유가 궁금합니다.
There was a problem hiding this comment.
화면 이동과 같은 버튼에서 collectLatest가 마지막 요청만 처리하게 되어 중복 동작을 예방할 수 있다고 알고 있어 collectLatest을 사용했어요!
There was a problem hiding this comment.
음 선택된 아이템을 바꾸는 동기로직은 바로 호출되는 순간 바로 수행되어서, 요청을 받는 중간(suspend 될 때)에 새로운 값이 수집되면 이전 작업을 버리는 collectLatest는 의미가 없다고 생각하는데 타마의 생각은 어떠신가요?
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함수를 사용하신 이유가 궁금해요
There was a problem hiding this comment.
해당 부분도 다른 부분 처럼 Icon 함수를 사용하도록 수정했습니다
| Scaffold( | ||
| modifier = modifier.fillMaxSize(), | ||
| containerColor = Color.White, | ||
| ) { |
There was a problem hiding this comment.
슬렉에서 말씀하신 빨간줄이 여기군용.
Scaffold를 사용하고 있는데 content에서 PaddingValues를 사용하지 않고 있어서 빨간줄 뜨는걸로 알고 있습니다.
There was a problem hiding this comment.
오잉
제 화면에서 이 부분은 에러가 나지 않는데, 혹시 에러가 나나요?!
There was a problem hiding this comment.
에러는 아니고 경고창이 보입니당. innerPadding으로 LazyColumn의 padding에 값을 주는 것도 좋을 것 같네요
| 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로 하신 이유가 있으실까요?
There was a problem hiding this comment.
해당 영역에 나중에 앱 정보와 같은 내용이 들어갈 수 있겠다고 생각했는데, 아직 고려하지 않아도 될 것 같네요😅
| // 라인업 리스트 | ||
| when (lineupUiState) { | ||
| is LineupUiState.Success -> { | ||
| val lineups = lineupUiState.lineups.getLineupItems() |
There was a problem hiding this comment.
여기 프로퍼티가 아니라 getLineupItems함수로 해야했던 이유가 있었을까요?(진짜 모름)
There was a problem hiding this comment.
LineupItemUiModel data class를 확인하면, 오늘이 라인업의 날짜와 같은 날인 지 판단하는 로직이 있습니다!
해당 로직은 이 함수를 호출할 때(UI가 이 데이터를 그리는 시점에) 계산이 되어야하는데요. 그래서 getLineupItems 함수를 사용했습니다.
There was a problem hiding this comment.
그런데 구조를 다시 생각해봤을 때, ui에서 계산 로직을 불러는 과정이 어색하게 느껴져 viewModel 내부로 계산 로직을 옮겼습니다~
| 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와 텍스트를 하나의 컴포저블로 묶어서 사이즈 관리를 하면 수월하지 않을까 합니다!
There was a problem hiding this comment.
헉,, 예리하시네요😯
바로 수정했습니다. .width(IntrinsicSize.Max) 이 속성도 알게되었네요!
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeLineupHeader.kt (2)
35-35: padding 표기를 간소화할 수 있습니다.현재 네 방향 모두 동일한
16.dp값을 사용하고 있어서,.padding(16.dp)로 간단하게 표기할 수 있습니다. 이렇게 하면 코드가 더 간결해지고 의도(모든 방향에 동일한 여백)가 명확해집니다.참고: 기존 리뷰 코멘트에서도 같은 내용이 언급되었습니다.
🔎 제안하는 수정
modifier = modifier .fillMaxWidth() - .padding(start = 16.dp, top = 16.dp, end = 16.dp, bottom = 16.dp), + .padding(16.dp),
51-51: 이 padding은 터치 영역 확장을 위해 유용합니다.기존 리뷰에서 이 4.dp padding의 필요성에 대한 질문이 있었는데, 실제로는 중요한 역할을 합니다.
존재 이유:
- 터치 타겟 확장: 이 padding이 없으면 클릭 가능한 영역이 텍스트와 아이콘의 정확한 경계로만 제한됩니다
- 접근성 개선: 특히 작은 아이콘(12.dp)의 경우, 추가 padding이 없으면 사용자가 정확히 터치하기 어려울 수 있습니다
- Material Design 가이드라인: 권장 최소 터치 타겟 크기는 48x48dp인데, 현재 텍스트+아이콘만으로는 이에 미치지 못할 가능성이 높습니다
참고: 시각적으로는 차이가 크지 않지만, 실제 사용성(터치 용이성)에는 명확한 차이가 있습니다. 현재 4.dp는 적절한 값으로 보이며, 유지하는 것을 권장합니다.
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeFestivalInfo.kt (1)
24-43: 구현이 깔끔합니다.컴포넌트 구조가 명확하고, 타이포그래피와 색상 사용이 디자인 시스템과 일관성 있게 적용되어 있습니다.
이전 리뷰에서 언급된 것처럼, 각
Text에 개별적으로padding(horizontal = 20.dp)를 적용하는 대신Column에서 한 번에 패딩을 주는 방식도 고려해볼 수 있습니다. 이렇게 하면 패딩 값 변경 시 한 곳만 수정하면 되어 유지보수가 용이해집니다.app/src/main/java/com/daedan/festabook/presentation/home/component/HomePosterList.kt (1)
80-97: 애니메이션 적용 방식이 효율적입니다.
graphicsLayer를 사용하여 scale과 alpha 애니메이션을 적용한 것은 좋은 선택입니다. 이 방식은 recomposition을 트리거하지 않고 RenderNode 레벨에서 처리되어 성능상 이점이 있습니다.이전 리뷰에서 언급된
cardBackground공통 컴포저블 사용도 일관성 측면에서 고려해볼 만합니다.app/src/main/java/com/daedan/festabook/presentation/main/MainActivity.kt (1)
166-173: Flow 기반 이벤트 수집으로의 전환이 잘 적용되었습니다.
repeatOnLifecycle(Lifecycle.State.STARTED)을 사용한 lifecycle-aware 수집 패턴이 올바르게 구현되어 있습니다.이전 리뷰에서 언급된
collectLatest사용에 대해: 네비게이션 이벤트와 같은 일회성 이벤트의 경우,collectLatest보다collect가 더 적합할 수 있습니다.
collectLatest: 새 값이 emit되면 이전 처리를 취소합니다. 빠른 연속 emit 시 중간 값이 무시될 수 있습니다.collect: 모든 emit된 값을 순차적으로 처리합니다.현재
navigateToScheduleEvent는SharedFlow로extraBufferCapacity = 1이 설정되어 있어 이벤트 손실 가능성은 낮지만, 의미상collect가 더 명확할 수 있습니다. 다만 현재 구현도 실질적으로 문제가 되지는 않습니다.app/src/main/java/com/daedan/festabook/presentation/home/component/HomeLineupItem.kt (2)
72-79: Divider 구현 방식 개선을 고려해보세요.이전 리뷰에서 언급된 것처럼,
Box를 사용한 커스텀 divider 대신HorizontalDivider사용을 고려해볼 수 있습니다.현재 구현은 동작에 문제가 없지만,
HorizontalDivider를 사용하면:
- Material Design 표준 컴포넌트와 일관성 유지
- 테마 변경 시 자동 대응
- 코드 의도가 더 명확해짐
다만 현재 디자인이 고정 너비(75.dp)를 요구하므로, 커스텀 구현이 필요할 수 있습니다. 이 경우 별도의
Divider컴포저블로 추출하면 재사용성과 유지보수성이 향상됩니다.
84-94: LazyRow 구현이 적절합니다.
items함수를 사용한 리스트 렌더링이 올바르게 구현되어 있습니다.horizontalArrangement으로 간격을 설정하고,contentPadding으로 좌우 여백을 처리한 점이 좋습니다.이전 리뷰에서 언급된 패딩 외부 제어 건은, 현재 구조에서는
HomeLineupItem내부에서 일관되게 관리하는 것이 더 응집력 있어 보입니다. 다만 디자인 시스템에서 화면별 마진 규칙이 다르다면 외부 제어를 고려해볼 수 있습니다.app/src/main/java/com/daedan/festabook/presentation/home/component/HomeScreen.kt (2)
147-149: 하단 여백에 Spacer 사용을 권장합니다.이전 리뷰에서 언급된 것처럼, 다른 곳에서는
Spacer를 사용하고 있는데 여기서는Box를 사용하고 있습니다. 일관성을 위해Spacer를 사용하는 것이 좋습니다.🔎 제안
// 하단 여백 추가 item { - Box(modifier = Modifier.padding(bottom = 60.dp)) + Spacer(modifier = Modifier.height(60.dp)) }
57-64: Scaffold의 PaddingValues를 적용해야 합니다.이전 리뷰에서 언급된 것처럼,
Scaffold의 content 람다에서 제공되는PaddingValues를 사용하지 않아 Lint 경고가 발생할 수 있습니다. 이 padding은 시스템 UI(status bar, navigation bar 등)를 고려한 값입니다.현재
HomeFragment에서 edge-to-edge가 적용되지 않아 문제가 없을 수 있지만, 향후 edge-to-edge UI 지원 시 필요합니다.🔎 수정 제안
Scaffold( modifier = modifier.fillMaxSize(), containerColor = Color.White, - ) { + ) { innerPadding -> LazyColumn( modifier = - Modifier.fillMaxSize() + Modifier + .fillMaxSize() + .padding(innerPadding) ) {app/src/main/java/com/daedan/festabook/presentation/home/component/HomeHeader.kt (1)
53-56: Icon 사용에 대한 이전 리뷰 코멘트를 참고하세요.이전 리뷰에서
parkjiminnnn님이Image대신Icon함수 사용에 대해 질문하셨는데, 이는 타당한 지적입니다.Icon을 사용해야 하는 이유:
- 의미론적 정확성:
Icon은 기능적/장식적 아이콘을 위해 설계되었으며, dropdown 화살표는 명확히 이 범주에 속합니다- tint 지원:
Icon은tint파라미터를 통해 색상을 쉽게 변경할 수 있어, 테마 변경이나 상태에 따른 색상 조정이 용이합니다- Material Design 가이드: Material Design에서는 UI 액션을 나타내는 아이콘에
Icon사용을 권장합니다Image를 써야 하는 경우:
- 복잡한 일러스트레이션이나 사진
- contentScale 조정이 필요한 경우
- 색상 변경이 불필요한 장식적 이미지
현재 dropdown 아이콘은
Icon으로 변경하는 것이 더 적절합니다.🔎 제안하는 수정
-import androidx.compose.foundation.Image +import androidx.compose.material3.Icon - Image( + Icon( painter = painterResource(id = R.drawable.ic_dropdown), contentDescription = stringResource(id = R.string.home_navigate_to_explore_desc), + tint = FestabookColor.black )
🧹 Nitpick comments (9)
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeLineupHeader.kt (1)
45-68: clickable의 indication 동작을 명시적으로 지정하는 것을 권장합니다.현재
clickable수정자가 기본 파라미터(indication, interactionSource)를 사용하고 있어서, Material 기본 ripple 효과가 적용됩니다.문제점:
- 의도한 UX가 명확하지 않음 - ripple이 필요한지, 없어야 하는지 코드만으로는 알 수 없습니다
- 디자인 시스템과의 일관성 확인 필요 - Festabook의 다른 클릭 가능한 요소들과 동일한 피드백을 제공해야 합니다
대안:
ripple 효과가 필요한 경우: 명시적으로 작성하여 의도를 분명히 합니다
.clickable( onClick = onScheduleClick, interactionSource = remember { MutableInteractionSource() }, indication = rememberRipple() )ripple 효과가 불필요한 경우 (텍스트+아이콘이 시각적 피드백 충분):
.clickable( onClick = onScheduleClick, interactionSource = remember { MutableInteractionSource() }, indication = null )디자인 명세나 다른 유사한 UI 요소들을 참고하여 어떤 방식이 적합한지 확인해보시는 것을 권장합니다.
app/src/main/java/com/daedan/festabook/presentation/home/HomeFragment.kt (2)
11-13: 사용되지 않는 import 정리가 필요합니다.
RecyclerView와FragmentHomeBindingimport가 남아있지만,onCreateView에서ComposeView를 직접 반환하므로 더 이상 사용되지 않는 것으로 보입니다.🔎 사용되지 않는 import 제거 제안
import android.os.Bundle import android.view.LayoutInflater import android.view.View import android.view.ViewGroup import androidx.compose.ui.platform.ComposeView import androidx.compose.ui.platform.ViewCompositionStrategy import androidx.fragment.app.viewModels import androidx.lifecycle.ViewModelProvider -import androidx.recyclerview.widget.RecyclerView import com.daedan.festabook.R -import com.daedan.festabook.databinding.FragmentHomeBinding import com.daedan.festabook.di.fragment.FragmentKey
25-26: Compose 마이그레이션 중 BaseFragment의 layoutId 재정의 검토HomeFragment가
onCreateView를 오버라이드하여ComposeView를 직접 반환하고 있어, 정의한layoutId프로퍼티가 실제로 사용되지 않는 상황입니다.BaseFragment의
onCreateView메서드는DataBindingUtil.inflate(inflater, layoutId, container, false)코드로 layoutId를 활용하는데, HomeFragment에서 super 호출 없이 onCreateView를 완전히 재정의했기 때문입니다. 따라서 현재 상태에서는 layoutId 선언이 가시적 불일치를 만들고 있습니다.선택 가능한 개선 방향:
- 옵션 1: Compose 전용 BaseFragment를 별도로 생성하여 layoutId 추상 프로퍼티를 제거 → 구조적 명확성 높음, 추가 클래스 필요
- 옵션 2: BaseFragment를 일단 유지하되, Compose 마이그레이션이 완료된 후 정리 → 현재 진행 중인 점진적 마이그레이션에 부담 적음, 기술 부채 발생 가능성
현재 상태로 유지해도 기능적 문제는 없으나, 향후 유사한 Compose 프래그먼트가 추가될 경우 BaseFragment 설계를 재고하는 것을 권장합니다.
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeScreen.kt (3)
27-27: 사용되지 않는 import가 있습니다.
collectLatest가 import되어 있지만 실제로 사용되지 않고 있습니다.HomeScreen에서는collectAsState를 사용하고 있으므로 이 import는 제거해도 됩니다.🔎 제안
import com.daedan.festabook.presentation.theme.FestabookColor -import kotlinx.coroutines.flow.collectLatest import java.time.LocalDate
137-143: Loading/Error 상태 처리가 필요합니다.현재
LineupUiState.Loading과LineupUiState.Error케이스가 빈 구현으로 남아있습니다. PR 설명에서 언급된 것처럼, 각 섹션별 로딩 상태 표시(skeleton 등)에 대한 결정이 필요합니다.최소한 Error 상태에서는 사용자에게 피드백을 제공하는 것이 좋습니다. 예를 들어:
- Retry 버튼이 있는 에러 메시지
- Snackbar를 통한 에러 알림
TODO 주석을 남겨두신 것으로 보아 인지하고 계신 부분이지만, 추후 구현이 필요합니다.
Loading/Error UI 구현을 도와드릴까요? 아니면 이 작업을 추적하기 위한 이슈를 생성해드릴까요?
66-74: FestivalUiState 조건부 렌더링에 대한 제안입니다.각
item블록마다if (festivalUiState is FestivalUiState.Success)를 반복 체크하고 있습니다. 이 패턴이 여러 번 반복되면 코드가 다소 verbose해질 수 있습니다.대안으로 smart cast를 활용하거나, Success 상태일 때만 전체 content를 렌더링하는 방식을 고려해볼 수 있습니다:
when (festivalUiState) { is FestivalUiState.Success -> { // 모든 festival 관련 item들을 여기서 렌더링 } is FestivalUiState.Loading -> { /* skeleton */ } is FestivalUiState.Error -> { /* error UI */ } }다만 현재 구조가 PR 설명에서 언급된 "각 섹션별 로딩 상태" 구현을 염두에 둔 것이라면, 현재 접근 방식이 더 유연할 수 있습니다.
app/src/main/java/com/daedan/festabook/presentation/home/HomeViewModel.kt (1)
39-51: 에러 처리 개선을 고려해볼 수 있습니다.현재
onFailure에서 에러를FestivalUiState.Error에 전달만 하고 있습니다. 프로덕션 환경에서는 다음을 고려해볼 수 있습니다:
- 로깅 추가: Timber 등을 사용한 에러 로깅
- 에러 분류: 네트워크 에러, 서버 에러 등 구분하여 사용자에게 적절한 메시지 제공
- 재시도 메커니즘: Retry 로직 추가
현재 마이그레이션 단계에서는 기존 동작을 유지하는 것이 우선이므로, 이는 향후 개선 사항으로 남겨두셔도 됩니다.
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeHeader.kt (2)
15-15: 사용되지 않는 import를 제거하세요.
androidx.compose.ui.graphics.Colorimport가 코드에서 실제로 사용되지 않고 있습니다. 코드에서는FestabookColor.black만 사용되므로, 이 import는 불필요합니다. 사용되지 않는 import는 코드를 읽는 사람에게 혼란을 줄 수 있고, 빌드 시간에도 미세하게 영향을 줄 수 있으므로 제거하는 것이 좋습니다.🔎 제안하는 수정
-import androidx.compose.ui.graphics.Color
32-37: 불필요한 Box 래퍼를 제거하여 컴포지션 계층을 최적화하세요.현재 Box가 단일 Row만 감싸고 있으며, 패딩 적용 외에 다른 역할을 하지 않습니다. 이런 경우 Box를 제거하고 패딩을 Row에 직접 적용하거나, 외부 modifier에 적용하는 것이 더 효율적입니다.
이유:
- 불필요한 컴포지션 레이어는 recomposition 시 약간의 오버헤드를 발생시킵니다
- 코드가 더 간결해지고 의도가 명확해집니다
🔎 제안하는 수정
옵션 1: Row에 직접 패딩 적용
- Box( - modifier = - modifier - .fillMaxWidth() - .padding(horizontal = 16.dp) - ) { - Row( - modifier = Modifier.clickable { onExpandClick() }, + Row( + modifier = modifier + .fillMaxWidth() + .padding(horizontal = 16.dp) + .clickable { onExpandClick() }, verticalAlignment = Alignment.CenterVertically, ) { // ... content } - }옵션 2: 외부에서 modifier를 통해 제어
- Box( - modifier = - modifier - .fillMaxWidth() - .padding(horizontal = 16.dp) - ) { - Row( - modifier = Modifier.clickable { onExpandClick() }, + Row( + modifier = modifier.clickable { onExpandClick() }, verticalAlignment = Alignment.CenterVertically, ) {그리고 호출하는 쪽에서 필요한 padding과 fillMaxWidth를 적용합니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/src/main/java/com/daedan/festabook/presentation/home/HomeFragment.ktapp/src/main/java/com/daedan/festabook/presentation/home/HomeViewModel.ktapp/src/main/java/com/daedan/festabook/presentation/home/component/HomeArtistItem.ktapp/src/main/java/com/daedan/festabook/presentation/home/component/HomeFestivalInfo.ktapp/src/main/java/com/daedan/festabook/presentation/home/component/HomeHeader.ktapp/src/main/java/com/daedan/festabook/presentation/home/component/HomeLineupHeader.ktapp/src/main/java/com/daedan/festabook/presentation/home/component/HomeLineupItem.ktapp/src/main/java/com/daedan/festabook/presentation/home/component/HomePosterList.ktapp/src/main/java/com/daedan/festabook/presentation/home/component/HomeScreen.ktapp/src/main/java/com/daedan/festabook/presentation/main/MainActivity.ktapp/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt
🧰 Additional context used
🧬 Code graph analysis (5)
app/src/main/java/com/daedan/festabook/presentation/home/component/HomePosterList.kt (1)
app/src/main/java/com/daedan/festabook/presentation/common/component/CoilImage.kt (1)
CoilImage(16-39)
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeArtistItem.kt (1)
app/src/main/java/com/daedan/festabook/presentation/common/component/CoilImage.kt (1)
CoilImage(16-39)
app/src/main/java/com/daedan/festabook/presentation/home/HomeFragment.kt (1)
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeScreen.kt (1)
HomeScreen(31-47)
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeScreen.kt (6)
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeHeader.kt (1)
HomeHeader(26-59)app/src/main/java/com/daedan/festabook/presentation/home/component/HomePosterList.kt (1)
HomePosterList(25-99)app/src/main/java/com/daedan/festabook/presentation/home/component/HomeFestivalInfo.kt (1)
HomeFestivalInfo(18-43)app/src/main/java/com/daedan/festabook/presentation/common/TextUtil.kt (1)
formatFestivalPeriod(5-15)app/src/main/java/com/daedan/festabook/presentation/home/component/HomeLineupHeader.kt (1)
HomeLineupHeader(26-70)app/src/main/java/com/daedan/festabook/presentation/home/component/HomeLineupItem.kt (1)
HomeLineupItem(33-98)
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeLineupItem.kt (1)
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeArtistItem.kt (1)
HomeArtistItem(22-52)
🔇 Additional comments (7)
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeLineupHeader.kt (1)
26-30: 전체적인 구조가 깔끔합니다.Composable 함수의 시그니처와 레이아웃 구조가 Compose 모범 사례를 잘 따르고 있습니다:
modifier파라미터를 선택적으로 받아 외부에서 추가 스타일 지정 가능- 콜백(
onScheduleClick)을 통한 명확한 이벤트 처리- 단일 책임 원칙을 따르는 컴포넌트 구조
Also applies to: 39-43
app/src/main/java/com/daedan/festabook/presentation/home/component/HomePosterList.kt (1)
30-38: 무한 스크롤 구현이 적절합니다.
Int.MAX_VALUE를 활용한 무한 스크롤 패턴이 잘 구현되어 있습니다.initialPage계산 시posterUrls.size로 나머지 연산을 통해 정확한 시작 위치를 보장하는 점이 좋습니다.다만 한 가지 엣지 케이스를 고려해보면,
posterUrls가 비어있을 때 Line 30에서 early return으로 처리되지만, 만약 런타임에 리스트가 비었다가 다시 채워지는 경우rememberPagerState가 이전 상태를 유지할 수 있습니다. 현재 구현에서는 early return으로 인해 이 문제가 발생하지 않지만, 향후 동적 리스트 변경이 필요하다면key파라미터 활용을 검토해보세요.app/src/main/java/com/daedan/festabook/presentation/home/HomeFragment.kt (1)
37-38: ViewCompositionStrategy 선택이 적절합니다.
DisposeOnViewTreeLifecycleDestroyed는 Fragment와 함께 사용할 때 권장되는 전략입니다. Fragment의 view lifecycle과 Composition의 lifecycle을 올바르게 연결하여 메모리 누수를 방지합니다.app/src/main/java/com/daedan/festabook/presentation/home/component/HomeArtistItem.kt (1)
22-61: 컴포저블과 객체의 네이밍에 대한 참고사항입니다.
HomeArtistItem이름이 composable 함수(Line 22)와 object(Line 54) 양쪽에서 사용되고 있습니다. Kotlin에서는 가능하지만, 일부 개발자에게 혼란을 줄 수 있습니다.몇 가지 대안을 제안드립니다:
- 현재 유지: Compose에서 이 패턴은 종종 사용됩니다 (예:
Card,Button등 Material 컴포넌트)- Object 이름 변경:
HomeArtistItemDefaults,HomeArtistItemStyle등으로 명확히 구분현재 구현도 동작에 문제가 없으며, 팀 내 컨벤션에 따라 결정하시면 됩니다.
shape 정의에서
bottomStartPercent = 5로 비대칭 모서리를 만든 것은 의도적인 디자인으로 보이며, 잘 구현되어 있습니다.app/src/main/java/com/daedan/festabook/presentation/home/HomeViewModel.kt (2)
24-32: LiveData에서 Flow로의 마이그레이션이 잘 되었습니다.StateFlow와 SharedFlow로의 전환이 올바르게 구현되어 있습니다:
- StateFlow: UI 상태 관리에 적합하며, 항상 최신 값을 가지고 있어 Compose의
collectAsState와 잘 어울립니다.- SharedFlow: 일회성 이벤트에 적합하며,
replay = 0, extraBufferCapacity = 1설정으로 이벤트 손실을 방지하면서도 구독 전 이벤트가 재생되지 않도록 했습니다.
asStateFlow()와asSharedFlow()를 통해 외부에 read-only로 노출한 점도 좋습니다.
53-55: tryEmit 사용이 적절합니다.
tryEmit은 suspend가 아닌 컨텍스트에서 SharedFlow에 값을 emit할 때 사용됩니다.extraBufferCapacity = 1이 설정되어 있으므로 버퍼가 가득 차지 않는 한 즉시 성공합니다.다만 혹시 emit이 실패할 경우를 대비하고 싶다면 반환값을 확인할 수 있습니다:
fun navigateToScheduleClick() { val emitted = _navigateToScheduleEvent.tryEmit(Unit) if (!emitted) { // 로깅 또는 대체 처리 } }현재 use case에서는 실패 확률이 매우 낮으므로 현재 구현도 충분합니다.
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeHeader.kt (1)
61-68: Preview 함수 구현이 좋습니다.Preview 함수가 best practice를 잘 따르고 있습니다:
private가시성으로 외부 노출 방지showBackground = true로 프리뷰 가시성 확보- 현실적인 샘플 데이터 제공 ("가천대학교")
이런 프리뷰는 IDE에서 UI 검증을 빠르게 할 수 있어 개발 효율을 높여줍니다.
| Row( | ||
| modifier = Modifier.clickable { onExpandClick() }, | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
클릭 가능한 영역에 시각적 피드백과 접근성 정보를 추가하세요.
현재 clickable modifier가 적용되어 있지만, 두 가지 개선이 필요합니다:
1. 시각적 피드백 부재:
indication 파라미터가 명시되지 않아 Material 3의 기본 ripple 효과가 적용되지 않을 수 있습니다. 사용자가 탭했을 때 시각적 피드백이 없으면 인터랙션이 등록되었는지 확인하기 어렵습니다.
2. 접근성 정보 부족:
스크린 리더 사용자를 위한 semantic 정보가 없습니다. 이 요소가 버튼 역할을 한다는 것과, 클릭 시 어떤 동작이 일어나는지 명시해야 합니다.
🔎 제안하는 수정
+import androidx.compose.foundation.interaction.MutableInteractionSource
+import androidx.compose.material.ripple.rememberRipple
+import androidx.compose.ui.semantics.Role
+
Row(
- modifier = Modifier.clickable { onExpandClick() },
+ modifier = Modifier.clickable(
+ onClick = onExpandClick,
+ role = Role.Button,
+ interactionSource = remember { MutableInteractionSource() },
+ indication = rememberRipple(bounded = true)
+ ),
verticalAlignment = Alignment.CenterVertically,
) {또는 더 간단하게, Material 3의 Surface를 사용하여 자동으로 ripple과 상태를 처리할 수도 있습니다.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeHeader.kt
around lines 38-41, the Row has a clickable modifier but lacks visual feedback
and accessibility semantics; update the Modifier to provide a ripple indication
(e.g., set indication = rememberRipple() or use the default Material surface)
and include accessibility semantics by marking the element as a button role and
supplying a clear contentDescription/label (or use Modifier.semantics to set
role = Role.Button and a descriptive text like "Expand header") so screen
readers and sighted users receive proper feedback; alternatively wrap the
content in a Material Surface which handles ripple and state automatically.
| style = FestabookTypography.displayLarge.copy( | ||
| platformStyle = PlatformTextStyle(includeFontPadding = false), | ||
| lineHeight = 34.sp |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "build.gradle*" -type f | head -20Repository: festabook/android
Length of output: 102
🏁 Script executed:
cat -n app/src/main/java/com/daedan/festabook/presentation/home/component/HomeHeader.kt | sed -n '30,60p'Repository: festabook/android
Length of output: 1191
🏁 Script executed:
grep -r "androidx.compose.ui.text.style" . --include="*.kt" --include="*.gradle*" | head -20Repository: festabook/android
Length of output: 338
🏁 Script executed:
cat app/build.gradle.kts | grep -i "compose\|accompanist" -A 1 -B 1Repository: festabook/android
Length of output: 637
🏁 Script executed:
grep -r "FestabookTypography" . --include="*.kt" -lRepository: festabook/android
Length of output: 756
🏁 Script executed:
cat -n build.gradle.kts | head -50Repository: festabook/android
Length of output: 571
🏁 Script executed:
find . -name "libs.versions.toml" -o -name "gradle.properties" | xargs ls -laRepository: festabook/android
Length of output: 206
🏁 Script executed:
cat gradle/libs.versions.toml | grep -i "compose" -A 2 -B 2Repository: festabook/android
Length of output: 2161
🏁 Script executed:
cat app/src/main/java/com/daedan/festabook/presentation/theme/FestabookTypography.ktRepository: festabook/android
Length of output: 2301
🏁 Script executed:
grep -r "PlatformTextStyle\|LineHeightStyle" . --include="*.kt" --include="*.gradle*"Repository: festabook/android
Length of output: 359
🌐 Web query:
Compose 2025.09 PlatformTextStyle deprecation LineHeightStyle replacement
💡 Result:
Short answer: don’t rely on PlatformTextStyle for line-height behavior — use TextStyle.lineHeight together with LineHeightStyle to control vertical alignment and trimming. PlatformTextStyle (and its includeFontPadding flag) was originally marked compatibility-only, later undeprecated for migration, but the recommended approach for controlling extra spacing is to set lineHeight and lineHeightStyle on Text/TextStyle. [1][2][3]
Minimal migration example:
-
Old (compat/migration)
platformStyle = PlatformTextStyle(includeFontPadding = false) -
New (recommended)
Text(
"Hello",
style = LocalTextStyle.current.copy(
lineHeight = 20.sp,
lineHeightStyle = LineHeightStyle(
alignment = LineHeightStyle.Alignment.Center,
trim = LineHeightStyle.Trim.None
)
)
)
References:
- TextStyle API (shows lineHeight and lineHeightStyle fields). [1]
- Android Developers blog / Medium post describing includeFontPadding history and recommending lineHeightStyle. [2][3]
PlatformTextStyle을 제거하되, lineHeight는 유지하면서 LineHeightStyle을 함께 사용하세요.
PlatformTextStyle의 includeFontPadding 플래그는 호환성 목적으로만 존재하던 API이며, 현재 권장되는 방식은 lineHeight와 LineHeightStyle을 함께 사용하여 텍스트 라인 높이와 수직 정렬을 제어하는 것입니다.
현재 코드의 문제점:
platformStyle = PlatformTextStyle(includeFontPadding = false)는 Android 구 버전 호환성 해결을 위한 임시 API로, 더 이상 권장되지 않습니다lineHeight = 34.sp는 displayLarge의 기본 fontSize(24.sp)를 넘어 의도적으로 큰 라인 높이를 설정한 것으로, 이 값은 보존되어야 합니다
권장 방식:
+import androidx.compose.ui.text.style.LineHeightStyle
+
Text(
text = schoolName,
style = FestabookTypography.displayLarge.copy(
- platformStyle = PlatformTextStyle(includeFontPadding = false),
- lineHeight = 34.sp
+ lineHeight = 34.sp,
+ lineHeightStyle = LineHeightStyle(
+ alignment = LineHeightStyle.Alignment.Center,
+ trim = LineHeightStyle.Trim.Both
+ )
),
color = FestabookColor.black,
)lineHeight를 제거하면 안 되는 이유는, 현재 설정된 34.sp가 UI 디자인의 의도적인 부분이기 때문입니다. LineHeightStyle은 lineHeight가 설정된 상태에서 그 높이 내에서 텍스트의 정렬 방식(상단/중앙/하단)과 불필요한 패딩 제거 여부를 제어하는 역할을 합니다.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| style = FestabookTypography.displayLarge.copy( | |
| platformStyle = PlatformTextStyle(includeFontPadding = false), | |
| lineHeight = 34.sp | |
| Text( | |
| text = schoolName, | |
| style = FestabookTypography.displayLarge.copy( | |
| lineHeight = 34.sp, | |
| lineHeightStyle = LineHeightStyle( | |
| alignment = LineHeightStyle.Alignment.Center, | |
| trim = LineHeightStyle.Trim.Both | |
| ) | |
| ), | |
| color = FestabookColor.black, | |
| ) |
🤖 Prompt for AI Agents
In
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeHeader.kt
around lines 44 to 46, remove the PlatformTextStyle(includeFontPadding = false)
entry, preserve lineHeight = 34.sp, and add a LineHeightStyle to control
vertical alignment and padding; specifically replace the platformStyle usage
with lineHeightStyle = LineHeightStyle(alignment =
LineHeightStyle.Alignment.Center, trim = LineHeightStyle.Trim.None) so the
explicit 34.sp lineHeight is kept while using the modern API to control vertical
alignment and trimming.
There was a problem hiding this comment.
@oungsi2000 네, 감사합니다! 제안드린 방식대로 PlatformTextStyle을 제거하고 LineHeightStyle을 사용하시면, 더 현대적이고 권장되는 Compose API를 활용하면서도 의도한 UI 레이아웃을 그대로 유지하실 수 있습니다.
혹시 적용 중 추가 질문이 있으시면 언제든 말씀해 주세요! 😊
| package com.daedan.festabook.presentation.home.component | ||
|
|
||
| import androidx.compose.foundation.clickable | ||
| import androidx.compose.foundation.interaction.MutableInteractionSource |
There was a problem hiding this comment.
사용되지 않는 import를 제거해주세요.
MutableInteractionSource를 import했지만 실제로 코드 내에서 사용되지 않고 있습니다. 현재는 clickable 수정자에서 기본 동작을 사용하고 있어서 필요하지 않은 import입니다.
🔎 제안하는 수정
import androidx.compose.foundation.clickable
-import androidx.compose.foundation.interaction.MutableInteractionSource
import androidx.compose.foundation.layout.Arrangement📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import androidx.compose.foundation.interaction.MutableInteractionSource | |
| import androidx.compose.foundation.clickable | |
| import androidx.compose.foundation.layout.Arrangement |
🤖 Prompt for AI Agents
In
app/src/main/java/com/daedan/festabook/presentation/home/component/HomeLineupHeader.kt
around line 4, the import
androidx.compose.foundation.interaction.MutableInteractionSource is unused;
remove that import line and run a quick compile or IDE-organize-imports to
ensure no other references remain and to keep imports tidy.
| // homeViewModel.festivalUiState.observe(viewLifecycleOwner) { state -> | ||
| // when (state) { | ||
| // is FestivalUiState.Error -> { | ||
| // showErrorSnackBar(state.throwable) | ||
| // Timber.w( | ||
| // state.throwable, | ||
| // "${this::class.simpleName}: ${state.throwable.message}", | ||
| // ) | ||
| // } | ||
| // | ||
| // FestivalUiState.Loading -> { | ||
| // binding.tvSettingCurrentUniversityNotice.text = "" | ||
| // } | ||
| // | ||
| // is FestivalUiState.Success -> { | ||
| // binding.tvSettingCurrentUniversity.text = state.organization.universityName | ||
| // } | ||
| // } | ||
| // } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find HomeViewModel file
fd -t f "HomeViewModel" --extension ktRepository: festabook/android
Length of output: 199
🏁 Script executed:
# Check SettingFragment for the commented code and surrounding context
cat -n app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt | sed -n '100,145p'Repository: festabook/android
Length of output: 2144
🏁 Script executed:
# Search for HomeViewModel's festivalUiState declaration to check its type
rg -n "festivalUiState" --type=kt -B 2 -A 2 | head -50Repository: festabook/android
Length of output: 87
🏁 Script executed:
# Look for StateFlow and Flow imports in HomeViewModel
rg -n "StateFlow|Flow" --type=kt app/src/main/java/com/daedan/festabook/presentation/home/ 2>/dev/null | head -20Repository: festabook/android
Length of output: 43
🏁 Script executed:
# Read HomeViewModel to check festivalUiState type and implementation
cat -n app/src/main/java/com/daedan/festabook/presentation/home/HomeViewModel.kt | head -80Repository: festabook/android
Length of output: 3196
🏁 Script executed:
# Search for festivalUiState declaration without type flag
rg -n "festivalUiState" -B 2 -A 2 | head -60Repository: festabook/android
Length of output: 6094
🏁 Script executed:
# Check if there are LiveData vs StateFlow usage patterns
rg -n "LiveData|StateFlow|MutableLiveData|MutableStateFlow" app/src/main/java/com/daedan/festabook/presentation/home/HomeViewModel.ktRepository: festabook/android
Length of output: 559
🏁 Script executed:
# Check SettingFragment class declaration and imports
head -50 app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt | grep -E "class SettingFragment|import.*Fragment|import.*lifecycleScope"Repository: festabook/android
Length of output: 297
🏁 Script executed:
# Verify if lifecycleScope is being used elsewhere in SettingFragment
rg -n "lifecycleScope" app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.ktRepository: festabook/android
Length of output: 43
StateFlow를 LiveData 방식으로 관찰할 수 없어 현재 학교 정보가 표시되지 않음
현재 코드는 StateFlow인 festivalUiState를 LiveData의 .observe() 메서드로 접근하려고 하기 때문에, 주석 처리된 옵저버가 재활성화되더라도 작동하지 않습니다. 사용자가 설정 화면에서 현재 연결된 학교를 확인할 수 없는 실질적인 기능 손실입니다.
문제 분석:
-
API 불일치: HomeViewModel의
festivalUiState는 이미 StateFlow로 마이그레이션되었지만, SettingFragment의 코드는 여전히 LiveData 패턴(.observe())을 사용합니다. 이 둘은 호환되지 않습니다. -
일관성 부재: 같은 Fragment 내에서
settingViewModel은 LiveData 옵저버를 사용하고 있는데,homeViewModel만 다른 패턴으로 처리되어 있습니다. 이는 코드 유지보수를 어렵게 합니다. -
마이그레이션 미완료: HomeScreen(Compose)은 이미
collectAsState()로 StateFlow를 올바르게 수집하고 있습니다. SettingFragment도 같은 방식으로 통일해야 합니다.
권장 해결책:
Fragment 환경에서 StateFlow를 안전하게 수집하려면 lifecycle-aware 방식이 필수입니다. lifecycleScope.launch와 repeatOnLifecycle을 활용하면 Fragment가 화면에 보이는 동안만 수집이 진행되고, 자동으로 정리되어 메모리 누수를 방지할 수 있습니다.
viewLifecycleOwner.lifecycleScope.launch {
viewLifecycleOwner.repeatOnLifecycle(Lifecycle.State.STARTED) {
homeViewModel.festivalUiState.collect { state ->
when (state) {
is FestivalUiState.Error -> {
showErrorSnackBar(state.throwable)
Timber.w(
state.throwable,
"${this::class.simpleName}: ${state.throwable.message}",
)
}
FestivalUiState.Loading -> {
binding.tvSettingCurrentUniversityNotice.text = ""
}
is FestivalUiState.Success -> {
binding.tvSettingCurrentUniversity.text = state.organization.universityName
}
}
}
}
}이 방식의 장점:
- 안전함: Lifecycle을 자동으로 추적하여 메모리 누수 방지
- 일관성: HomeScreen과 같은 Flow 기반 패턴 사용
- 유지보수성: 최신 Android 아키텍처 권장사항 준수
주석 처리된 코드를 이 패턴으로 교체하면 사용자가 설정 화면에서 현재 학교 정보를 다시 볼 수 있습니다.
네! 말씀하신 것 처럼 Scaffold에 containerColor을 지정하지 않으면 MaterialTheme의 기본 색상을 따라가더라구요! |
oungsi2000
left a comment
oungsi2000
left a comment
There was a problem hiding this comment.
마이그레이션 고생하셨습니다 !
포스터 스크롤이 더 부드러워진 느낌도 드네요.
전반적으로 넘 잘해주셨지만 몇 가지 버그가 있어 RC 드립니다 !
모든 상태(festival, lineup 등등)이 성공 상태일 때, 홈 화면 전체를 보여주는 게 좋을 까요? 아니면 각 항목별로 로딩 상태일 때의 액션(스켈레톤 등)을 추가하는 게 좋을까요?
저도 분리하는 것이 좋을 것 같습니다 !
현재는 포스터에 스켈레톤이 없어 포스터가 로딩되기 전에 라인업이 로딩이 완료되면
포스터의 영역이 사라졌다 생기는 현상이 있어 조금 부자연스러운 것 같아요 !
스켈레톤으로 메꿔주면 훨씬 좋을 것 같습니다.
| object HomeArtistItem { | ||
| val ArtistImage = RoundedCornerShape( | ||
| topStartPercent = 50, | ||
| topEndPercent = 50, |
There was a problem hiding this comment.
private 을 다는건 어떻게 생각하시나요?
| private fun HomeArtistItemPreview() { | ||
| HomeArtistItem( | ||
| artistName = "실리카겔", |
There was a problem hiding this comment.
이건 TMI일 수도 있는데, Theme으로 감싸주지 않고 Preview를 실행하는건 실제 사용할 때와 다르게 동작할 수 있어 주의해야 하는 것으로 알고있어요 !
| style = FestabookTypography.displayLarge.copy( | ||
| platformStyle = PlatformTextStyle(includeFontPadding = false), | ||
| lineHeight = 34.sp |
| Spacer(modifier = Modifier.width(4.dp)) | ||
|
|
There was a problem hiding this comment.
festabookSpacing을 써도 될 것 같아요 !
| contentDescription = null, | ||
| tint = FestabookColor.gray400, | ||
| modifier = Modifier.size(12.dp), |
There was a problem hiding this comment.
기존 XML에서도 contentDescription이 없었을까요?
Compose는 XML과 달리 UI의 Sementic을 중요시하는 것으로 알고 있어서 없다면 추가해줘도 좋을 것 같아요!
|
|
||
| Spacer(modifier = Modifier.height(4.dp)) | ||
|
|
||
| Box( |
|
|
||
| // 아티스트 가로 리스트 | ||
| LazyRow( | ||
| contentPadding = PaddingValues(horizontal = 16.dp), |
There was a problem hiding this comment.
요기도 festabookSpacing 써줘도 좋을 것 같아요 !
| contentDescription = null, | ||
| modifier = Modifier.fillMaxSize(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
중요
기존에는 포스터 클릭 시 확대 가능한 Dialog를 띄웠는데요 (엄재웅님 라이브러리) 해당 기능이 누락된 것 같습니다 !
There was a problem hiding this comment.
기존에 사용하던 GetStream/photoview-android 라이브러리는 XML 기반 라이브러리이기 때문에, Compose환경에서는 AndroidView로 감싸서 사용해야하더라구요.
그래서 다른 라이브러리 사용을 슬랙-#기능제안에서 제안하고자합니다!
There was a problem hiding this comment.
중요
포스터 이미지가 없는 축제에 들어갔을 때, (dev서버의 가천대학교)
기존에는 자동으로 에러 이미지가 채워졌는데,
현재는 포스터 영역이 완전히 사라집니다 !
There was a problem hiding this comment.
저도 이 부분에 대해서 고민해봤는데요.
포스터 이미지가 없는데, 포스터 영역이 화면에 있는 게 맞나? 이런 생각도 들더라구요.
어떻게 생각하시나요!?
@parkjiminnnn 의 의견도 궁금합니다!
There was a problem hiding this comment.
뭔가 없는 이미지라도 ui는 그대로 채우는게 자연스러울거라고 생각하는데 에러이미지로 없는 이미지를 채우자니 뭔가 좀 딱딱한 느낌이라..
Festabook 로고가 들어간 이미지라도 간단하게 만들어서 채우는게 보기 좋지 않을까 합니다.
홈 화면을 구성하는 주요 컴포저블(`HomeHeader`, `HomePosterList` 등)의 파라미터 네이밍을 정리하고, 디자인 시스템 및 커스텀 Modifier를 적용하여 코드 일관성을 높였습니다.
- **`HomeHeader.kt` 수정:**
- 파라미터 이름을 `schoolName`에서 `universityName`으로 변경하여 도메인 모델과의 일관성을 맞췄습니다.
- 드롭다운 아이콘을 `Image`에서 `Icon`으로 변경하고, `FestabookColor`와 고정 크기(24.dp)를 적용했습니다.
- **`HomePosterList.kt` 수정:**
- 포스터 카드에 커스텀 Modifier인 `cardBackground`를 적용하여 배경 스타일을 개선했습니다.
- **`HomeScreen.kt` 수정:**
- `HomeHeader` 호출 시 변경된 파라미터 명(`universityName`)을 반영했습니다.
- 리스트 하단의 여백 처리를 위해 의미상 더 적절한 `Spacer`를 사용하도록 변경했습니다.
- **`HomeLineupHeader.kt` 수정:**
- 일정 클릭 영역(`Row`)에서 불필요한 `padding(4.dp)`을 제거하여 레이아웃을 정돈했습니다.
parkjiminnnn
left a comment
parkjiminnnn
left a comment
There was a problem hiding this comment.
고생 많으셨습니다. 몇 가지 코멘트만 확인 부탁드려요~
| class HomeViewModel @Inject constructor( | ||
| private val festivalRepository: FestivalRepository, |
| 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), | ||
| ) | ||
| } | ||
| } |
| Scaffold( | ||
| modifier = modifier.fillMaxSize(), | ||
| containerColor = Color.White, | ||
| ) { |
There was a problem hiding this comment.
에러는 아니고 경고창이 보입니당. innerPadding으로 LazyColumn의 padding에 값을 주는 것도 좋을 것 같네요
| ) { | ||
| // 날짜 + 배지 영역 | ||
| Row( | ||
| modifier = Modifier.padding(horizontal = 16.dp), |
There was a problem hiding this comment.
아하 무슨 말씀인지 이해했습니다~ 저도 이대로 가는 방법밖에는 안떠오르네요
oungsi2000
left a comment
oungsi2000
left a comment
There was a problem hiding this comment.
라이브러리 사용 제안이 엄재웅님 라이브러리 때문이었군요..!
근데 제안주신 라이브러리에 피치 줌 기능이 들어가 있는지는 제가 확인하지 못했네요.
이것만 반영해주시면 될 것 같아요!
고생하셨습니다 타마~~
#️⃣ 이슈 번호
🛠️ 작업 내용
🙇🏻 중점 리뷰 요청
주의할 부분
논의할 부분
1. 홈 화면 로딩 관련
모든 상태(festival, lineup 등등)이 성공 상태일 때, 홈 화면 전체를 보여주는 게 좋을 까요? 아니면 각 항목별로 로딩 상태일 때의 액션(스켈레톤 등)을 추가하는 게 좋을까요?
2. FestabookColor.white
해당 속성이 기존에 사용하던 배경색(진짜 쌩 화이트)가 아니라 새롭게 지정한 디자인 시스템의 white 입니다. 따라서 화면의 배경색을 지정할 때,
FestabookColor.white를 사용하지 못하고 Color.white를 사용해야합니다. 이 점에 대해 따로 배경색을 추가하는 방향은 어떤 지 논의하고 싶습니다.
📸 이미지 첨부 (Optional)
Summary by CodeRabbit
릴리스 노트
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.