[FEAT/#99] 장소 상세 페이지 더미 데이터 구현 #100

Roel4990 · 2025-01-21T00:52:22Z

Related issue 🛠

closed [FEAT] 장소 상세 페이지 더미 데이터 구현 #99

Work Description ✏️

장소 상세 페이지 더미 데이터 구현

Screenshot 📸

placeDetailComplete.mp4

To Reviewers 📢

chattymin

고생하셨습니다!
확인 부탁드려요~

chattymin · 2025-01-21T01:50:02Z

app/src/main/java/com/spoony/spoony/presentation/main/MainScreen.kt

@@ -52,7 +52,8 @@ fun MainScreen(
            )

            placeDetailNavGraph(
-                paddingValues = innerPadding
+                paddingValues = innerPadding,
+                navController = navigator.navController


p1) navController를 매개변수로 넘겨주면 좋지 않습니다!
이유를 생각해보시고, 헷갈린다면 편하게 말씀주세요 :)

navController 을 파라미터로 넘기지 않고 아래와 같은 방식으로 전달하게 되면 이동 로직이 한곳에 유지되므로 코드를 쉽게 유지관리할 수 있고 실수로 개별 화면에 앱의 특정 Route 를 자유롭게 허용하지 않음으로써 버그를 방지할 수 있다는 장점이 있습니다.

navController.navigateTo('home')

참고 :
https://developer.android.com/codelabs/basic-android-kotlin-compose-navigation?hl=ko#4

참고해서 수정하겠습니다!

수정했습니다!

순환호출!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

chattymin · 2025-01-21T08:04:53Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/PlaceDetailRoute.kt

+    LaunchedEffect(key1 = true) {
+        viewModel.getPost(postId)
+    }


p1) viewModel에서의 init을 통해 구현한 방법과 LaunchedEffect를 활용한 방법 중 어떤 방법이 더 적합할까요?

LaunchedEffect 사용이 적합한 경우

네트워크 호출이 화면이나 컴포저블 상태에 강하게 의존하는 경우.

postId와 같은 동적 파라미터가 자주 변경될 수 있는 경우.

특정 컴포저블에서만 호출이 필요할 때.

ViewModel의 init 사용이 적합한 경우

고정된 초기 데이터를 항상 불러와야 하는 경우.

네트워크 호출이 컴포저블과 독립적으로 실행되어야 하는 경우.

여러 컴포저블이 동일한 데이터를 공유해야 할 때.

저희의 상황에서는
동적인 파라미터를 처리하거나 컴포저블 상태와 데이터 로딩을 밀접하게 동기화할 필요는 없다고 생각하고,
ViewModel의 초기화 과정에서 데이터를 준비하고 컴포저블과 독립적으로 관리하는 것이 더 좋아보이기 때문에
ViewModel 에 init 으로 관리하는것이 더 좋아보입니다..!

chattymin · 2025-01-21T08:05:08Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/PlaceDetailRoute.kt

-                    onBackButtonClick = {},
-                    onReportButtonClick = {}
+                    onBackButtonClick = navigateToMap,
+                    onReportButtonClick = { navigateToReport() }


Suggested change

onReportButtonClick = { navigateToReport() }

onReportButtonClick = navigateToReport

수정하겠습니다!

수정했습니다!

했다매!!!!!!!!!!!!!

p1) onBackButton은 navigatUp으로 구현하면 좋을 것 같아요~

진짜 수정함..!

chattymin · 2025-01-21T08:06:01Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/PlaceDetailViewModel.kt

    var _state: MutableStateFlow<PlaceDetailState> = MutableStateFlow(PlaceDetailState())
    val state: StateFlow<PlaceDetailState>
        get() = _state

-    fun useSpoon() {
+    fun getPost(postId: Int) {


p3) 지금은 괜찮지만, 추후 서버통신이 실패했을 경우를 대비해 onFailure도 작성해주세요~!

수정했습니다!

chattymin · 2025-01-21T08:07:04Z

...src/main/java/com/spoony/spoony/presentation/placeDetail/navigation/PlaceDetailNavigation.kt

+    navOptions: NavOptions? = null,
+    postId: Int,
+    userId: Int


p1) 필수가 위로가고, 옵셔널이 밑으로 가게 순서 변경부탁드립니다!

수정했습니다!

…ail-dummy # Conflicts: # app/src/main/java/com/spoony/spoony/presentation/placeDetail/PlaceDetailRoute.kt

chattymin

한번만 더 확인 부탁드려요!

chattymin · 2025-01-21T09:28:53Z

app/src/main/java/com/spoony/spoony/presentation/main/MainScreen.kt

@@ -53,7 +54,7 @@ fun MainScreen(

            placeDetailNavGraph(
                paddingValues = innerPadding,
-                navController = navigator.navController
+                navigateToReport = { navigator.navController.navigateToReport() }


p2) 다른 화면 이동의 예시 파일처럼 MainNavigation 내부에서 함수를 만들어서 구현해주세용~

수정했습니다!

chattymin · 2025-01-21T09:30:23Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/PlaceDetailRoute.kt

-                    onBackButtonClick = {},
-                    onReportButtonClick = {}
+                    onBackButtonClick = navigateToMap,
+                    onReportButtonClick = { navigateToReport() }


했다매!!!!!!!!!!!!!

chattymin · 2025-01-21T09:30:45Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/PlaceDetailRoute.kt

-                    onBackButtonClick = {},
-                    onReportButtonClick = {}
+                    onBackButtonClick = navigateToMap,
+                    onReportButtonClick = { navigateToReport() }


p1) onBackButton은 navigatUp으로 구현하면 좋을 것 같아요~

chattymin · 2025-01-21T09:31:43Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/PlaceDetailViewModel.kt

 ) : ViewModel() {
    var _state: MutableStateFlow<PlaceDetailState> = MutableStateFlow(PlaceDetailState())
    val state: StateFlow<PlaceDetailState>
        get() = _state

+    private val postId: Int = savedStateHandle.get<Int>("postId") ?: -1


p1) 이 변수는 state가 아니라 따로 관리하는 이유가 있나요?
p2) 저희는 직렬화를 사용하고 있기 때문에 String으로 key를 하는게 아니라 클래스 접근의 방식으로도 가능합니다. safety하게 해주세요!

수,,수정했습니다!

Hyobeen-Park · 2025-01-21T15:52:44Z

app/src/main/java/com/spoony/spoony/presentation/main/MainScreen.kt

+                navigateToReport = { navigator.navController.navigateToReport() },
+                navigateToUp = { navigator.navController.navigateUp() }


Suggested change

navigateToReport = { navigator.navController.navigateToReport() },

navigateToUp = { navigator.navController.navigateUp() }

navigateToReport = navigator.navController::navigateToReport,

navigateToUp = navigator.navController::navigateUp

P2: 짜잔!!

p2) MainNavigator class내부에 navigate 함수를 만드는건 어떨까요?
p3) navigateUp의 경우 navigateToUp이라는 네이밍은 어색한 것 같아요.

fun navigateToReport(navOptions: NavOptions = null) { navController.navigateToReport(navOptions) }

수정했습니다! 감사해요!

Hyobeen-Park · 2025-01-21T15:55:43Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/PlaceDetailViewModel.kt

+class PlaceDetailViewModel @Inject constructor(
+    private val postRepository: PostRepository,
+    savedStateHandle: SavedStateHandle
+) : ViewModel() {
    var _state: MutableStateFlow<PlaceDetailState> = MutableStateFlow(PlaceDetailState())


P1: 이거 private 안붙이면 state가 외부에서 수정가능해집니다!!

수정했습니다!

Hyobeen-Park · 2025-01-21T16:03:12Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/PlaceDetailViewModel.kt

+                    val currentPostEntity = (_state.value.postEntity as? UiState.Success)?.data
+                    if (currentPostEntity != null) {


P3: null 체크를 하시는 이유가 궁금합니다!

최대한 짧게 수정했습니다.

chattymin

고생하셨습니다! 마지막으로 한번만 부탁드려요 ㅜㅜ

chattymin · 2025-01-21T16:07:58Z

app/src/main/java/com/spoony/spoony/presentation/main/MainScreen.kt

+                navigateToReport = { navigator.navController.navigateToReport() },
+                navigateToUp = { navigator.navController.navigateUp() }


p2) MainNavigator class내부에 navigate 함수를 만드는건 어떨까요?
p3) navigateUp의 경우 navigateToUp이라는 네이밍은 어색한 것 같아요.

fun navigateToReport(navOptions: NavOptions = null) { navController.navigateToReport(navOptions) }

chattymin · 2025-01-21T16:08:20Z

app/src/main/java/com/spoony/spoony/presentation/placeDetail/PlaceDetailRoute.kt

-                    onMenuItemClick = onReportButtonClick
+                    onMenuItemClick = { menu ->
+                        when (menu) {
+                            "신고하기" -> {


p2) 추후 늘어날 수 있기에 미리 enum으로 만들어둬도 좋을 것 같아요 :)

수정했습니다!

…ail-dummy

chattymin

고생했습니다!

Hyobeen-Park

어푸푸~~!!🚀🚀

angryPodo

수고하셨습니다~

[FEAT/#99] 장소 상세 페이지 더미 데이터 처리

ad4e8c9

Roel4990 added 🧡세홍🧡 🧡세홍🧡 FEAT✨ 새로운 기능 구현 labels Jan 21, 2025

Roel4990 requested review from chattymin, Hyobeen-Park and angryPodo January 21, 2025 00:52

Roel4990 self-assigned this Jan 21, 2025

Roel4990 requested a review from a team as a code owner January 21, 2025 00:52

Roel4990 linked an issue Jan 21, 2025 that may be closed by this pull request

[FEAT] 장소 상세 페이지 더미 데이터 구현 #99

Closed

1 task

chattymin requested changes Jan 21, 2025

View reviewed changes

Roel4990 added 3 commits January 21, 2025 18:04

[FEAT/#99] 파라미터 navController -> navigateTo~ 로 넘기도록 수정

8dc30e2

[FEAT/#99] ktLinkFormat 처리

41ad482

[FEAT/#99] 코드리뷰 수정사항 반영

a766e5c

Roel4990 requested a review from chattymin January 21, 2025 09:15

Merge remote-tracking branch 'origin/develop' into feat/#99-place-det…

00c0986

…ail-dummy # Conflicts: # app/src/main/java/com/spoony/spoony/presentation/placeDetail/PlaceDetailRoute.kt

chattymin requested changes Jan 21, 2025

View reviewed changes

Roel4990 added 3 commits January 21, 2025 18:55

[FEAT/#99] navigate 부분 수정

9e982db

[FEAT/#99] ktLintFormat 처리

66df335

[FEAT/#99] postArgs 값 state 에서 관리

afc3327

Roel4990 requested a review from chattymin January 21, 2025 15:40

Hyobeen-Park reviewed Jan 21, 2025

View reviewed changes

chattymin requested changes Jan 21, 2025

View reviewed changes

Roel4990 added 3 commits January 22, 2025 02:12

[FEAT/#99] 코드리뷰 수정사항 반영

68c8968

[FEAT/#99] ktLintFormat 처리

d93752b

Merge remote-tracking branch 'origin/develop' into feat/#99-place-det…

b96e929

…ail-dummy

Roel4990 requested review from Hyobeen-Park and chattymin January 21, 2025 17:14

chattymin approved these changes Jan 21, 2025

View reviewed changes

Hyobeen-Park approved these changes Jan 21, 2025

View reviewed changes

angryPodo approved these changes Jan 21, 2025

View reviewed changes

Roel4990 merged commit 9bcd09c into develop Jan 21, 2025
1 check passed

	onReportButtonClick = { navigateToReport() }
	onReportButtonClick = navigateToReport

		navigateToReport = { navigator.navController.navigateToReport() },
		navigateToUp = { navigator.navController.navigateUp() }

		val currentPostEntity = (_state.value.postEntity as? UiState.Success)?.data
		if (currentPostEntity != null) {

[FEAT/#99] 장소 상세 페이지 더미 데이터 구현 #100

[FEAT/#99] 장소 상세 페이지 더미 데이터 구현 #100

Conversation

Roel4990 commented Jan 21, 2025

Related issue 🛠

Work Description ✏️

Screenshot 📸

To Reviewers 📢

chattymin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaunchedEffect 사용이 적합한 경우

ViewModel의 init 사용이 적합한 경우

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chattymin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chattymin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chattymin left a comment

Choose a reason for hiding this comment

Hyobeen-Park left a comment

Choose a reason for hiding this comment

angryPodo left a comment

Choose a reason for hiding this comment