[Feat] PlaceDetailActivity Compose 마이그레이션#25
Conversation
장소의 상세 정보를 표시하는 화면을 구현하고, 이미지 슬라이더 및 확대/축소 기능을 적용했습니다. 또한, 긴 설명 텍스트를 펼쳐볼 수 있도록 기존 텍스트 컴포넌트를 개선했습니다.
- **`PlaceDetailScreen.kt` 추가:**
- 장소 이미지, 제목, 운영 시간, 위치, 호스트 정보를 표시하는 상세 화면 UI를 구현했습니다.
- `Landscapist` 라이브러리를 활용하여 이미지 페이징 및 핀치 줌(Zoomable) 기능이 포함된 이미지 상세 보기 다이얼로그를 구성했습니다.
- 설명 텍스트 클릭 시 내용이 확장되거나 축소되는 애니메이션 인터랙션을 추가했습니다.
- **`URLText.kt` 수정:**
- 텍스트 영역 클릭 이벤트를 처리할 수 있도록 `onClick` 파라미터와 탭 제스처 감지 로직을 추가했습니다.
- **빌드 설정 수정:**
- 이미지 로딩 및 줌 기능을 위해 `landscapist-coil3`, `landscapist-zoomable` 라이브러리 의존성을 추가했습니다.
`URLText` 컴포넌트에서 URL 클릭 시 부모의 클릭 이벤트가 함께 발생하는 문제를 해결하고, URL 탐지 정규식을 변경했습니다.
- **`URLText.kt` 수정:**
- Android의 `Patterns.WEB_URL` 의존성을 제거하고, 한글 등을 포함할 수 있는 커스텀 정규식(`WEB_REGEX`)으로 교체하여 Kotlin Regex API를 사용하도록 변경했습니다.
- `detectTapGestures` 내부 로직을 수정하여, URL 영역을 클릭했을 때는 `uriHandler.openUri`만 실행되고, 그 외 영역을 클릭했을 때만 `onClick` 콜백이 호출되도록 개선했습니다.
기존 XML 및 DataBinding 기반의 장소 상세 화면을 Jetpack Compose로 마이그레이션하고, 상태 관리 로직을 개선했습니다.
- **`PlaceDetailActivity.kt` 리팩토링:**
- `AppCompatActivity`를 `ComponentActivity`로 변경하고, `setContent` 블록 내에서 `PlaceDetailScreen`을 호출하도록 수정했습니다.
- 기존의 `ActivityPlaceDetailBinding`, `ViewPager2`, `Adapter` 등 View 시스템 관련 코드를 모두 제거했습니다.
- **`PlaceDetailViewModel.kt` 수정:**
- `LiveData`로 관리되던 `placeDetail` 상태를 `StateFlow`로 변경하여 데이터 스트림을 개선했습니다.
- 장소 상세 정보의 이미지가 비어있을 경우, 기본 `ImageUiModel`을 리스트에 포함하도록 로직을 추가했습니다.
- **`PlaceDetailScreen.kt` 수정:**
- 컴포저블의 파라미터를 `PlaceDetailUiModel`에서 `PlaceDetailUiState`로 변경했습니다.
- `UiState`의 상태(`Success`, `Loading`, `Error`)에 따라 `PlaceDetailContent`, `LoadingStateScreen`, `EmptyStateScreen`을 분기하여 렌더링하도록 구조를 변경했습니다.
`PlaceDetailViewModel` 테스트 코드에서 `getOrAwaitValue` 유틸리티 함수 대신 `value` 프로퍼티를 직접 참조하는 방식으로 검증 로직을 단순화했습니다.
- **`PlaceDetailViewModelTest.kt` 수정:**
- 불필요해진 `getOrAwaitValue` import 구문을 제거했습니다.
- `placeDetail`의 상태를 검증하는 모든 테스트 케이스에서 `getOrAwaitValue()` 호출을 `value` 프로퍼티 접근으로 대체했습니다.
장소 상세 화면의 이미지 로딩 로직을 공통 컴포넌트(`FestabookImage`)로 교체하여 코드를 간소화하고, 더 이상 사용되지 않는 View 기반 어댑터 코드를 정리했습니다.
- **`PlaceDetailScreen.kt` 수정:**
- `Landscapist` 라이브러리의 `CoilImage`를 직접 호출하던 코드를 제거하고, `FestabookImage` 컴포넌트로 대체했습니다.
- 이미지 상세 보기 다이얼로그에 `isZoomable = true` 옵션을 적용하여 줌 기능을 유지했습니다.
- **미사용 코드 삭제:**
- Compose 마이그레이션 후 불필요해진 `PlaceImageViewPagerAdapter.kt` 및 `PlaceImageViewPagerViewHolder.kt` 파일을 삭제했습니다.
|
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 📝 WalkthroughWalkthroughThis PR migrates the PlaceDetail feature from traditional View-based architecture with DataBinding to Jetpack Compose. The changes include adopting StateFlow over LiveData for state management, removing legacy ViewPager adapter components, and introducing a new Compose-based UI with enhanced URL handling capabilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@CodeRabbit 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
🤖 Fix all issues with AI agents
In
@app/src/main/java/com/daedan/festabook/presentation/placeDetail/component/PlaceDetailScreen.kt:
- Around line 369-378: The formattedDate function currently returns "null ~ ..."
when one parameter is null; update formattedDate(startTime: String?, endTime:
String?) to explicitly handle partial nulls: if both null return
stringResource(R.string.place_detail_default_time), if only startTime is null
return endTime, if only endTime is null return startTime, otherwise return
"$startTime ~ $endTime" (or use list join only when both non-null); ensure you
reference the formattedDate function and its startTime/endTime parameters and
keep the stringResource call for the default case.
- Around line 84-91: Replace Modifier.scrollable(...) on the Column with
Modifier.verticalScroll(scrollState) so the scroll offset is applied to the
content (locate the Column that currently uses modifier.scrollable(state =
scrollState, orientation = Orientation.Vertical) and swap to
Modifier.verticalScroll(scrollState)); also remove the now-unused import related
to Orientation/scrollable if it becomes unused.
- Around line 197-211: You’re invoking onPageUpdate(page) directly in the
HorizontalPager content which runs during composition (a side effect); remove
that call from the pager lambda and instead observe pagerState.currentPage in a
side-effect such as LaunchedEffect with snapshotFlow: create a LaunchedEffect
tied to pagerState (or pagerState) that uses snapshotFlow {
pagerState.currentPage }.distinctUntilChanged().collect { onPageUpdate(it) } so
onPageUpdate is only called when the page actually changes.
- Around line 331-339: The Image used as a back button in PlaceDetailScreen (the
Image composable with painterResource(R.drawable.btn_back_to_previous) and
onClick) currently has contentDescription = null; replace this with a meaningful
accessible description (e.g., use a string resource like R.string.back_button or
a localized string such as "Back") so screen readers announce its purpose;
update the Image's contentDescription to reference that string resource (or
accept a parameter passed into the composable) to ensure accessibility without
changing the click behavior.
🧹 Nitpick comments (5)
app/src/main/java/com/daedan/festabook/presentation/common/component/URLText.kt (1)
112-113: Consider matching URLs without explicit protocol prefix.The current regex requires an explicit protocol (
https?|ftp|file), which means common patterns likewww.example.comwon't be detected as URLs. If users paste or type URLs without the protocol prefix, they won't be clickable.♻️ Optional: Extended regex to catch www. prefixed URLs
private val WEB_REGEX = - """(https?|ftp|file)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;\uAC00-\uD7AF]*[-a-zA-Z0-9+&@#/%=~_|\uAC00-\uD7AF]""".toRegex() + """((https?|ftp|file)://|www\.)[-a-zA-Z0-9+&@#/%?=~_|!:,.;\uAC00-\uD7AF]*[-a-zA-Z0-9+&@#/%=~_|\uAC00-\uD7AF]""".toRegex()Note: If extending to
www.prefix, you'd also need to prependhttps://before passing touriHandler.openUri()for URLs that don't have a protocol.app/src/test/java/com/daedan/festabook/placeDetail/PlaceDetailViewModelTest.kt (1)
30-31: Consider removing InstantTaskExecutorRule if LiveData is fully migrated.
InstantTaskExecutorRuleis specifically for testingLiveDatasynchronously. With the migration toStateFlow, this rule may no longer be necessary. If no other tests in this class or the ViewModel use LiveData, consider removing it to clean up the test setup.app/src/main/java/com/daedan/festabook/presentation/placeDetail/PlaceDetailViewModel.kt (1)
42-48: Consider extracting default image logic to reduce duplication.The same pattern for injecting a default
ImageUiModelwhen images are empty appears in both theinitblock andloadPlaceDetail. This could be extracted into a helper function.♻️ Optional: Extract helper for consistent image list handling
private fun PlaceDetailUiModel.withDefaultImageIfEmpty(): PlaceDetailUiModel = if (images.isEmpty()) copy(images = listOf(ImageUiModel())) else thisThen use it:
init { if (receivedPlaceDetail != null) { - val placeDetailUiModel = - if (receivedPlaceDetail.images.isEmpty()) { - receivedPlaceDetail.copy(images = listOf(ImageUiModel())) - } else { - receivedPlaceDetail - } - _placeDetail.value = PlaceDetailUiState.Success(placeDetailUiModel) + _placeDetail.value = PlaceDetailUiState.Success( + receivedPlaceDetail.withDefaultImageIfEmpty() + ) } else if (place != null) { loadPlaceDetail(place.id) } }Also applies to: 59-66
app/src/main/java/com/daedan/festabook/presentation/placeDetail/PlaceDetailActivity.kt (2)
46-53: Consider wrapping content with FestabookTheme.With
enableEdgeToEdge(), you may need to ensure the Compose content is wrapped with your app's theme and handles system bar insets properly. ThePlaceDetailScreenshould be wrapped withFestabookThemefor consistent styling.♻️ Suggested theme wrapper
enableEdgeToEdge() setContent { + FestabookTheme { val placeDetailUiState by viewModel.placeDetail.collectAsStateWithLifecycle() PlaceDetailScreen( uiState = placeDetailUiState, onBackToPreviousClick = { finish() }, ) + } }Additionally, ensure
PlaceDetailScreenhandles window insets for edge-to-edge display (e.g., usingModifier.systemBarsPadding()orScaffoldwith proper insets).
55-55: Debug log may not capture meaningful state.This log statement executes immediately after
setContent, at which pointplaceDetail.valueis likely stillPlaceDetailUiState.Loading. Consider moving this to aLaunchedEffector removing it if it's only for debugging.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/com/daedan/festabook/presentation/common/component/URLText.ktapp/src/main/java/com/daedan/festabook/presentation/placeDetail/PlaceDetailActivity.ktapp/src/main/java/com/daedan/festabook/presentation/placeDetail/PlaceDetailViewModel.ktapp/src/main/java/com/daedan/festabook/presentation/placeDetail/adapter/PlaceImageViewPagerAdapter.ktapp/src/main/java/com/daedan/festabook/presentation/placeDetail/adapter/PlaceImageViewPagerViewHolder.ktapp/src/main/java/com/daedan/festabook/presentation/placeDetail/component/PlaceDetailScreen.ktapp/src/test/java/com/daedan/festabook/placeDetail/PlaceDetailViewModelTest.kt
💤 Files with no reviewable changes (2)
- app/src/main/java/com/daedan/festabook/presentation/placeDetail/adapter/PlaceImageViewPagerViewHolder.kt
- app/src/main/java/com/daedan/festabook/presentation/placeDetail/adapter/PlaceImageViewPagerAdapter.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run-PR-Test
🔇 Additional comments (5)
app/src/main/java/com/daedan/festabook/presentation/common/component/URLText.kt (1)
33-33: LGTM on the onClick fallback pattern.The addition of
onClickparameter with a sensible default allows parent components to handle taps on non-URL areas, fixing the issue where parent click handlers couldn't be invoked when tapping on text. The fallback via?: onClick()at line 86 correctly differentiates between URL taps and regular text taps.Also applies to: 86-86
app/src/test/java/com/daedan/festabook/placeDetail/PlaceDetailViewModelTest.kt (1)
68-70: LGTM on StateFlow test pattern.Direct
.valueaccess afteradvanceUntilIdle()is the correct pattern for testingStateFlowwithStandardTestDispatcher. The synchronous access is safe since all coroutines have completed.Also applies to: 88-90, 106-109, 129-132
app/src/main/java/com/daedan/festabook/presentation/placeDetail/PlaceDetailViewModel.kt (1)
34-38: LGTM on StateFlow migration.The migration from
MutableLiveDatatoMutableStateFlowwithPlaceDetailUiState.Loadingas the initial value is correct. StateFlow's non-null guarantee aligns well with the sealed class state pattern.app/src/main/java/com/daedan/festabook/presentation/placeDetail/component/PlaceDetailScreen.kt (1)
147-147: Verify ifbeyondViewportPageCount = 5is necessary.Preloading 5 pages beyond the viewport could increase memory usage, especially with zoomable images. The default value is typically sufficient unless there's a specific UX requirement for pre-caching multiple pages.
Also applies to: 201-201
app/src/main/java/com/daedan/festabook/presentation/placeDetail/PlaceDetailActivity.kt (1)
47-53: LGTM on Compose migration.The migration to
ComponentActivitywithsetContentandcollectAsStateWithLifecyclefollows best practices for lifecycle-aware state collection in Compose. The callback tofinish()foronBackToPreviousClickis appropriate.
app/src/main/java/com/daedan/festabook/presentation/placeDetail/component/PlaceDetailScreen.kt
Show resolved
Hide resolved
app/src/main/java/com/daedan/festabook/presentation/placeDetail/component/PlaceDetailScreen.kt
Show resolved
Hide resolved
| Image( | ||
| modifier = | ||
| modifier | ||
| .padding(top = festabookSpacing.paddingBody4) | ||
| .size(30.dp) | ||
| .clickable { onClick() }, | ||
| painter = painterResource(id = R.drawable.btn_back_to_previous), | ||
| contentDescription = null, | ||
| ) |
There was a problem hiding this comment.
Add accessibility content description for back button.
contentDescription = null prevents screen readers from announcing this interactive element. Users relying on accessibility services won't understand the button's purpose.
♿ Suggested fix
Image(
modifier =
modifier
.padding(top = festabookSpacing.paddingBody4)
.size(30.dp)
.clickable { onClick() },
painter = painterResource(id = R.drawable.btn_back_to_previous),
- contentDescription = null,
+ contentDescription = stringResource(R.string.content_description_back_button),
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@app/src/main/java/com/daedan/festabook/presentation/placeDetail/component/PlaceDetailScreen.kt
around lines 331 - 339, The Image used as a back button in PlaceDetailScreen
(the Image composable with painterResource(R.drawable.btn_back_to_previous) and
onClick) currently has contentDescription = null; replace this with a meaningful
accessible description (e.g., use a string resource like R.string.back_button or
a localized string such as "Back") so screen readers announce its purpose;
update the Image's contentDescription to reference that string resource (or
accept a parameter passed into the composable) to ensure accessibility without
changing the click behavior.
| @Composable | ||
| private fun formattedDate( | ||
| startTime: String?, | ||
| endTime: String?, | ||
| ): String = | ||
| if (startTime == null && endTime == null) { | ||
| stringResource(R.string.place_detail_default_time) | ||
| } else { | ||
| listOf(startTime, endTime).joinToString(" ~ ") | ||
| } |
There was a problem hiding this comment.
Handle partial null values in date formatting.
When only one of startTime or endTime is null, joinToString will produce strings like "null ~ 18:00" or "09:00 ~ null", which is not user-friendly.
💡 Suggested fix
@Composable
private fun formattedDate(
startTime: String?,
endTime: String?,
): String =
- if (startTime == null && endTime == null) {
+ when {
+ startTime == null && endTime == null ->
stringResource(R.string.place_detail_default_time)
- } else {
- listOf(startTime, endTime).joinToString(" ~ ")
+ startTime != null && endTime != null ->
+ "$startTime ~ $endTime"
+ startTime != null -> startTime
+ else -> endTime!!
}📝 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.
| @Composable | |
| private fun formattedDate( | |
| startTime: String?, | |
| endTime: String?, | |
| ): String = | |
| if (startTime == null && endTime == null) { | |
| stringResource(R.string.place_detail_default_time) | |
| } else { | |
| listOf(startTime, endTime).joinToString(" ~ ") | |
| } | |
| @Composable | |
| private fun formattedDate( | |
| startTime: String?, | |
| endTime: String?, | |
| ): String = | |
| when { | |
| startTime == null && endTime == null -> | |
| stringResource(R.string.place_detail_default_time) | |
| startTime != null && endTime != null -> | |
| "$startTime ~ $endTime" | |
| startTime != null -> startTime | |
| else -> endTime!! | |
| } |
🤖 Prompt for AI Agents
In
@app/src/main/java/com/daedan/festabook/presentation/placeDetail/component/PlaceDetailScreen.kt
around lines 369 - 378, The formattedDate function currently returns "null ~
..." when one parameter is null; update formattedDate(startTime: String?,
endTime: String?) to explicitly handle partial nulls: if both null return
stringResource(R.string.place_detail_default_time), if only startTime is null
return endTime, if only endTime is null return startTime, otherwise return
"$startTime ~ $endTime" (or use list join only when both non-null); ensure you
reference the formattedDate function and its startTime/endTime parameters and
keep the stringResource call for the default case.
장소 상세 화면의 스크롤이 올바르게 동작하도록 수정자를 변경하고, 프리뷰용 더미 텍스트를 일부 수정했습니다.
- **`PlaceDetailScreen.kt` 수정:**
- `Column`에 사용된 `scrollable` 수정자를 `verticalScroll`로 변경하여 세로 스크롤이 정상적으로 작동하도록 개선했습니다.
- 프리뷰에 사용되는 `uiState`의 설명(`description`) 텍스트 내 URL 오타(`htt` -> `http`) 및 띄어쓰기를 수정했습니다.
etama123
left a comment
etama123
left a comment
There was a problem hiding this comment.
몇 가지 수정하면 좋을 부분에 리뷰를 달았어요!
고생하셨습니다~ 밀러
| URLText( | ||
| modifier = | ||
| Modifier | ||
| .animateContentSize( | ||
| animationSpec = | ||
| spring( | ||
| dampingRatio = Spring.DampingRatioLowBouncy, | ||
| stiffness = Spring.StiffnessMedium, | ||
| ), | ||
| ).padding( | ||
| top = festabookSpacing.paddingBody3, | ||
| ), | ||
| onClick = { | ||
| isDescriptionExpand = !isDescriptionExpand | ||
| }, | ||
| text = | ||
| placeDetail.place.description | ||
| ?: stringResource(R.string.place_list_default_description), | ||
| style = FestabookTypography.bodySmall, | ||
| maxLines = | ||
| if (isDescriptionExpand) { | ||
| Int.MAX_VALUE | ||
| } else { | ||
| 1 | ||
| }, | ||
| overflow = TextOverflow.Ellipsis, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
설명 부분의 기본값이 펼쳐져 있는 상태로 변경되지 않았나요?
There was a problem hiding this comment.
엇..release 앱 확인해보니, 기본값은 닫힌 상태였습니다 !
There was a problem hiding this comment.
전에 구두로 논의만 하고 적용하지 않고 넘어가버렸군요😥
혹시 지금이라도 펼쳐진 형태로 변경하는 건 어떨까요?
| Row( | ||
| modifier = Modifier.padding(top = festabookSpacing.paddingBody4), | ||
| ) { | ||
| Icon( | ||
| painter = painterResource(R.drawable.ic_place_detail_clock), | ||
| contentDescription = stringResource(R.string.content_description_iv_clock), | ||
| ) | ||
|
|
||
| Text( | ||
| modifier = Modifier.padding(start = festabookSpacing.paddingBody1), | ||
| text = formattedDate(placeDetail.startTime, placeDetail.endTime), | ||
| style = FestabookTypography.bodySmall, | ||
| color = FestabookColor.gray500, | ||
| ) | ||
| } |
There was a problem hiding this comment.
반복되는 부분을 공통 컴포넌트로 분리하면 어떨까요?
There was a problem hiding this comment.
각각의 Icon 컴포저블과 modifier가 달라 분리를 해도 비슷한 것 같습니다. 다만 저 반복되는 부분을 하나로 묶어 보았습니다 !
refactor(PlaceDetail): 장소 상세 정보 UI 컴포넌트 분리
There was a problem hiding this comment.
음 이미지 아이콘과 modifier를 파라미터로 정하면, 각 아이템을 컴포저블로 바꿀 수 있지 않을까요? 아래의 형태처럼요!
데이터가 제각각이라 분리해도 비슷해 보일 수 있다는 점 공감합니다! 다만, 공통 컴포저블로 추출하면 PlaceDetailInfo는 로우 레벨의 UI 구현(Row, Icon, Text 조합)보다는 어떤 데이터를 보여줄 것인지에만 집중할 수 있고, 구조를 더 한 눈에 확인할 수 있어 전체적인 가독성이 훨씬 좋아질 것 같아요.
private fun PlaceDetailInfoItem(
@DrawableRes iconResId: Int,
contentDescription: String,
text: String,
modifier: Modifier = Modifier,
)There was a problem hiding this comment.
|
|
||
| when (uiState) { | ||
| is PlaceDetailUiState.Success -> { | ||
| PlaceDetailImageDialog( |
There was a problem hiding this comment.
해당 화면이 시스템 표시줄과 꽤 많은 영역이 겹쳐, 사진이 가려보여요!
툴바처럼 상단에 여백을 주는 것은 어떨까요?
There was a problem hiding this comment.
흠...혹시 사진이 가려보인다는 부분을 더 자세히 공유해주실 수 있으실까요? 저는 잘 체감이 안되서요 !
There was a problem hiding this comment.
| private fun BackToPreviousButton( | ||
| onClick: () -> Unit, | ||
| modifier: Modifier = Modifier, | ||
| ) { | ||
| Image( | ||
| modifier = | ||
| modifier | ||
| .padding(top = festabookSpacing.paddingBody4) | ||
| .size(30.dp) | ||
| .clickable { onClick() }, | ||
| painter = painterResource(id = R.drawable.btn_back_to_previous), | ||
| contentDescription = null, |
There was a problem hiding this comment.
버튼을 눌렀을 때가 아닌, 버튼 바깥의 여백부분을 눌렀을 때만 동작하네요. 확인 부탁드립니다!
There was a problem hiding this comment.
어라..? 이것도 제 핸드폰에서는 정상 작동하는데 혹시 공유해주실 수 있으실까요?
|
|
||
| class PlaceDetailViewModel @AssistedInject constructor( |
There was a problem hiding this comment.
AssistedInject를 클래스 단위로 옮겨도 좋을 것 같네용
There was a problem hiding this comment.
| val placeDetailUiModel = | ||
| if (receivedPlaceDetail.images.isEmpty()) { | ||
| receivedPlaceDetail.copy(images = listOf(ImageUiModel())) | ||
| } else { | ||
| receivedPlaceDetail | ||
| } | ||
| _placeDetail.value = PlaceDetailUiState.Success(placeDetailUiModel) | ||
| } else if (place != null) { |
There was a problem hiding this comment.
init {
receivedPlaceDetail?.let {
val placeDetailUiModel =
if (it.images.isEmpty()) it.copy(images = listOf(ImageUiModel())) else it
_placeDetail.value = PlaceDetailUiState.Success(placeDetailUiModel)
}
place?.let { loadPlaceDetail(it.id) }
}if문 분기처리가 많아지면 가독성이 떨어진다고 생각해서 이렇게 리팩토링을 해봤는데 어떠신가요?
There was a problem hiding this comment.
오 확실히 이전보다 가독성이 좋아진 것 같아요 ! 반영했습니다 !
refactor(PlaceDetail): PlaceDetailViewModel 초기화 로직 리팩토링
| var isDialogOpen by remember { mutableStateOf(false) } | ||
| var currentPage by remember { mutableIntStateOf(0) } |
There was a problem hiding this comment.
상태를 외부에서 들고 있게 하는건 어떠신가요?
There was a problem hiding this comment.
구조의 변경으로 currentPage는 사라졌습니다.
외부에서 PlaceDetailScreen을 호출한다고 했을 때,
외부에서 해당 상태들을 알면 오히려 불필요한 정보까지 알게 된다고 생각했습니다. (추상화의 이점 상실)
| } else { | ||
| listOf(startTime, endTime).joinToString(" ~ ") | ||
| } |
There was a problem hiding this comment.
string.xml 에 format_date가 있어요!
There was a problem hiding this comment.
| painter = painterResource(id = R.drawable.btn_back_to_previous), | ||
| contentDescription = null, |
There was a problem hiding this comment.
기존에도 contentDescription이 null이었을까용?
There was a problem hiding this comment.
settingFragment에도 달았던 내용인데 instantTaskExecutorRule 없어도 될 것 같아용
| .value | ||
| .let { it as PlaceDetailUiState.Success } |
There was a problem hiding this comment.
val actual =
placeDetailViewModel.placeDetail
.value
.let {
(it as? PlaceDetailUiState.Success)
?: fail { "PlaceDetailUiState 가 성공 상태가 아님" }
}.placeDetail
.noticesfail 함수를 사용하면 캐스팅 오류가 발생 했을 때 에러 로그를 확인하기 더 수월할 듯 한데 어떠신가요?
There was a problem hiding this comment.
오 이런 좋은 함수가 있었군요 ! 반영했습니다!
test(PlaceDetail): PlaceDetailViewModelTest 검증 로직 안정성 강화
장소 상세 정보(위치, 운영 시간 등)를 표시하는 UI 로직을 `PlaceDetailInfo` 컴포저블로 분리하여 코드 구조를 개선했습니다.
- **`PlaceDetailScreen.kt` 수정:**
- 기존 `PlaceDetailContent` 내부에 있던 장소 정보(주소, 시간, 전화번호) 관련 `Row` 및 `Text` 컴포저블들을 `PlaceDetailInfo`라는 새로운 컴포저블 함수로 추출했습니다.
- `PlaceDetailContent`는 `PlaceDetailInfo`를 호출하도록 변경하여 코드의 가독성과 재사용성을 높였습니다.
장소 상세 화면의 이미지 페이저(`Pager`)와 다이얼로그 페이저 간의 페이지 상태를 동기화하고, 상태 관리 로직을 개선했습니다.
- **`PlaceDetailScreen.kt` 수정:**
- 이미지 페이저의 `PagerState`를 `PlaceDetailScreen`의 `Success` 상태 분기 내에서 생성하도록 변경하여, 다이얼로그와 메인 화면 페이저가 동일한 상태를 공유하도록 수정했습니다.
- `PlaceDetailImageDialog`는 외부에서 생성된 `pagerState`를 받도록 변경하여, 다이얼로그가 열릴 때 현재 페이지를 정확히 표시하도록 개선했습니다.
- 메인 화면 페이저(`PlaceDetailImageContent`)의 페이지 변경 시 `scrollToPage`를 호출하도록 수정하고, 페이지가 완전히 변경되었을 때(`settledPage`)만 `onPageUpdate` 콜백이 호출되도록 `LaunchedEffect`를 추가했습니다. 이로써 불필요한 재구성을 줄였습니다.
`PlaceDetailViewModel`의 `@AssistedInject` 어노테이션 위치를 생성자에서 클래스 선언부로 이동하여 코드 스타일을 일관성 있게 수정했습니다.
`PlaceDetailViewModel`의 `init` 블록에서 `if` 조건문을 `let` 스코프 함수로 변경하여 코드를 간소화하고 가독성을 개선했습니다.
- **`PlaceDetailViewModel.kt` 수정:**
- `receivedPlaceDetail` 및 `place` 객체의 null 체크 로직을 `?.let` 블록으로 대체했습니다.
- `let` 블록 내에서 삼항 연산자를 사용하여 이미지가 비어있을 경우 기본 이미지 모델을 추가하는 로직을 간결하게 수정했습니다.
장소 상세 화면(`PlaceDetailScreen`)의 이미지 페이저 상태 관리 로직을 개선하고, 불필요한 코드를 정리했습니다.
- **`PlaceDetailScreen.kt` 수정:**
- 이미지 상세 보기 다이얼로그(`ImageDetailDialog`)가 열릴 때, 페이저의 현재 페이지를 유지하기 위해 사용하던 `currentPage` 상태 변수를 제거했습니다. `rememberPagerState`는 상태를 자동으로 유지하므로 불필요한 로직이었습니다.
- 장소 운영 시간 표시를 위해 하드코딩된 문자열 `" ~ "` 대신, 형식 문자열 리소스(`R.string.format_date`)를 사용하도록 변경하여 코드의 일관성을 높였습니다.
장소 상세 화면의 뒤로가기 버튼에 콘텐츠 설명(`contentDescription`)을 추가하여 접근성을 개선했습니다.
- **`PlaceDetailScreen.kt` 수정:**
- 뒤로가기 버튼(`Image`)의 `contentDescription`에 `stringResource`를 사용하여 "상세화면 나가기 아이콘"이라는 설명을 추가했습니다.
- **`strings.xml` 추가:**
- `content_description_exit_place_detail` 문자열 리소스를 새로 추가했습니다.
장소 상세 화면의 UI 코드를 정리하여 스타일 일관성을 개선했습니다.
- **`PlaceDetailScreen.kt` 수정:**
- 뒤로가기 버튼(`IcBackToPrevious`)의 `padding` Modifier 적용 순서를 변경하여 Z-Index가 올바르게 작동하도록 수정했습니다.
- `IcBackToPrevious` 컴포저블 내부에 적용되었던 불필요한 `padding(top)` 속성을 제거했습니다.
`PlaceDetailViewModelTest` 테스트 코드에서 더 이상 필요하지 않은 `InstantTaskExecutorRule` 관련 코드를 삭제했습니다.
- **`PlaceDetailViewModelTest.kt` 수정:**
- 불필요해진 `InstantTaskExecutorRule` import 구문을 제거했습니다.
- `@get:Rule` 어노테이션과 함께 선언된 `instantTaskExecutorRule` 프로퍼티를 삭제했습니다.
`PlaceDetailViewModel`의 단위 테스트에서 UI 상태를 검증하는 로직을 더 안전하게 수정했습니다.
- **`PlaceDetailViewModelTest.kt` 수정:**
- `PlaceDetailUiState.Success` 상태를 검증할 때, `as`를 사용한 강제 타입 캐스팅 대신 `as?`와 `fail` 함수를 조합하여 사용하도록 변경했습니다. 이를 통해 테스트가 예기치 않은 상태(예: `Error`)로 인해 실패할 경우, 더 명확한 실패 메시지를 제공하도록 개선했습니다.
etama123
left a comment
etama123
left a comment
There was a problem hiding this comment.
리뷰 반영 고생하셨습니다. 추가로 약간의 의견을 적어두었으니 확인해주시고 다시 리뷰 요청바랍니다🙂
장소 상세 화면의 정보 표시 UI를 개선하고 사용자 경험을 위해 상세 설명 섹션의 초기 상태를 변경했습니다.
- **`PlaceDetailScreen.kt` 수정:**
- 시간, 위치, 주최자 정보를 표시하는 중복된 `Row`와 `Icon`, `Text` 컴포저블을 `PlaceDetailInfoItem`이라는 새로운 컴포저블로 추출하여 코드 중복을 제거하고 재사용성을 높였습니다.
- 장소 상세 설명(`PlaceDetailDescriptionSection`)이 기본적으로 펼쳐진 상태로 보이도록 `isDescriptionExpand`의 초기값을 `true`로 변경했습니다.
장소 상세 화면의 뒤로가기 버튼이 상태 표시줄(Status Bar) 영역과 겹쳐 보이는 문제를 해결하고, 전체 화면(Edge-to-Edge) UI의 스타일을 명확하게 설정했습니다.
- **`PlaceDetailActivity.kt` 수정:**
- `enableEdgeToEdge` 호출 시 `SystemBarStyle.light`를 적용하여, 스크롤 시 상태 표시줄의 배경(scrim) 색상을 흰색으로 지정했습니다.
- 기존에 `setContent` 이전에 호출되던 `enableEdgeToEdge`를 `setContent` 내부로 이동시켜 UI 상태와 함께 관리되도록 수정했습니다.
- **`PlaceDetailScreen.kt` 수정:**
- 뒤로가기 버튼(`BackToPreviousButton`)에 `windowInsetsPadding(WindowInsets.statusBars)` Modifier를 추가하여, 상태 표시줄 높이만큼의 패딩을 동적으로 적용했습니다. 이로 인해 버튼이 상태 표시줄 아래에 올바르게 배치됩니다.
URL을 감지하는 정규식(`WEB_REGEX`)의 정확도를 높였습니다. URL 끝에 포함될 수 없는 마침표(.), 쉼표(,), 콜론(:), 세미콜론(;)과 같은 문자가 URL의 일부로 잘못 인식되는 문제를 해결했습니다.
- **`URLText.kt` 수정:**
- 기존 정규식의 마지막에 부정형 후방탐색 `(?<![.,:;])`을 추가했습니다.
- 이를 통해 URL이 해당 문장 부호로 끝나는 경우, 이를 URL에서 제외하여 링크의 정확성을 개선했습니다.
|
상태바랑 겹치는 버그 수정했습니다 |
3 participants
#️⃣ 이슈 번호
🛠️ 작업 내용
🙇🏻 중점 리뷰 요청
📸 이미지 첨부 (Optional)
mobizen_20260108_004750.mp4
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.