Feat/week04 compose #6

t1nm1ksun · 2024-05-03T09:50:00Z

📌 PR 요약

🌱 작업한 내용

4주차 compose 필수과제

🌱 PR 포인트

API를 통해 회원가입, 로그인 , 비밀번호 변경 구현
로그인이 되고나면 이전 스택을 지워 뒤로가기 버튼 클릭시 로그인으로 돌아가지 않도록 함

week04_compose.mp4

로그인 후 네비게이션바로 마이페이지 이동 시 인자 넘겨주지 않아서 꺼지는 문제 뷰모델 이용해서 해결

0se0

수고하셨습니다!

0se0 · 2024-05-05T09:09:04Z

app/src/main/java/com/sopt/now/compose/screen/ChangePasswordScreen.kt

+                            popUpTo(Routes.Password.route) {
+                                inclusive = true
+                            }
+                            launchSingleTop = true


launchSingleTop = true 이 코드는 어떤 일을 하는 것인지 궁금합니다!

이거 해당 페이지를 스택 맨위로 올려주는거에요!

0se0 · 2024-05-05T09:13:05Z

app/src/main/java/com/sopt/now/compose/screen/LoginScreen.kt

+                response: Response<ResponseLogInDto>
+            ) {
+                if (response.isSuccessful) {
+                    val memberId = response.headers()["Location"]!!.toInt()


여기서 non-null 연산자를 사용하신 이유가 궁금합니다!

0se0 · 2024-05-05T09:19:12Z

app/src/main/java/com/sopt/now/compose/screen/MyPageScreen.kt

+                    userId = response.body()!!.data.authenticationId
+                    userNickname = response.body()!!.data.nickname
+                    userPhone = response.body()!!.data.phone


널 처리 관련하여 좀 더 안전한 방법을 사용해보시는 것은 어떨까요?!

twogarlic · 2024-05-05T12:20:07Z

app/src/main/java/com/sopt/now/compose/screen/MyPageScreen.kt

+                if (response.isSuccessful) {
+                    userId = response.body()!!.data.authenticationId
+                    userNickname = response.body()!!.data.nickname
+                    userPhone = response.body()!!.data.phone


response.body()에 너무 많이 접근하는 것 같습니다! 줄이는 방법은 없을까요?

l2hyunwoo

안녕하세요 민석님. 안드로이드 파트 명예 OB 이현우입니다. 만나서 반갑습니다.

앞으로의 과제들에 대한 코드리뷰를 맡게 되었습니다. 고민해보시다가 궁금한 점이 있으시면 지식 공유방/디스코드 연락 해주세요. 대면/비대면 자유형식으로 추가 피드백 가능합니다.

우선 과제 어려우셨을텐데 빠르게 과제 수행해주신점 너무 좋습니다. 관련해서 추가적으로 피드백/질문 남긴 것들이 있습니다. 질문을 남긴 경우 코드에 문제가 있어서 질문이 남긴 것이 아니고 정말 민석님의 작성 의도가 궁금해서 남긴 것이니 이 점 참고해주시면 감사하겠습니다.

앞으로 잘 부탁드리겠습니다.

추가적으로 현재 상태에 따라서 화면을 그리는 컴포저블의 렌더링 과정/상태관리/UI와 ViewModel의 역할분리에 대해 이해가 필요한 것 같습니다. 해당 부분은 코드랩 혹은 안드로이드 공식 양질의 샘플들을 확인해보시면 좋을 것 같습니다.

l2hyunwoo · 2024-05-05T14:37:45Z

app/build.gradle

    androidTestImplementation 'androidx.compose.ui:ui-test-junit4'
    debugImplementation 'androidx.compose.ui:ui-tooling'
    debugImplementation 'androidx.compose.ui:ui-test-manifest'
+    implementation "androidx.navigation:navigation-compose:2.7.7"


중복선언입니다.

l2hyunwoo · 2024-05-05T14:40:04Z

app/src/main/java/com/sopt/now/compose/BottomBar.kt

+fun BottomBar(
+    navController: NavHostController,
+    modifier: Modifier = Modifier,
+    isLoggedIn: Boolean
+) {


Kotlin에서는 모든 선택적 패러미터(Optional Parameter, 디폴트 값이 주어진 패러미터)는 필수 패러미터 뒤에 배치되어야합니다. 그리고 컴포즈에서는 모든 선택적 패러미터 제일 위에 Modifier가 배치되어야 합니다...라고 코딩 컨벤션에는 적혀 있습니다.

필수는 아니지만 공통 룰을 지켜보면서 코드 가독성 올리는 것에 도움이 될 수 있을 것 같아서 공유드립니다.

Suggested change

fun BottomBar(

navController: NavHostController,

modifier: Modifier = Modifier,

isLoggedIn: Boolean

) {

fun BottomBar(

navController: NavHostController,

isLoggedIn: Boolean,

modifier: Modifier = Modifier,

) {

l2hyunwoo · 2024-05-05T14:41:31Z

app/src/main/java/com/sopt/now/compose/BottomBar.kt

+    if (isLoggedIn) {
+        val navBackStackEntry by navController.currentBackStackEntryAsState()
+        val currentRoute = navBackStackEntry?.destination?.route
+        NavigationBar(
+            modifier = modifier,
+            containerColor = Color.LightGray,
+        ) {
+            screens.forEach { screen ->
+
+                NavigationBarItem(
+                    label = {
+                        Text(text = screen.title!!)
+                    },
+                    icon = {
+                        Icon(imageVector = screen.icon!!, contentDescription = "")
+                    },
+                    selected = currentRoute == screen.route,
+                    onClick = {
+                        navController.navigate(screen.route)
+                    },
+                    colors = NavigationBarItemDefaults.colors(
+                        unselectedTextColor = Color.Gray, selectedTextColor = Color.White
+                    ),
+                )
+            }
+        }
+    }


만약 바텀바가 로그인된 상태 이외에도 보여야 한다라는 요구사항 변경이 들어오면 어떻게 될까요? 그리고 특정 조건에서도 보이지 않아야 한다라는 조건이 추가된다면요? 컴포넌트 단에서 이런 앱 내 기능 요구사항들을 알아야 할 필요가 있을까요?

l2hyunwoo · 2024-05-05T14:42:00Z

app/src/main/java/com/sopt/now/compose/BottomBar.kt

+    val screens = listOf(
+        BottomNavItem.Home, BottomNavItem.Search, BottomNavItem.MyPage
+    )


(아주 사소한 리뷰) 이 리스트는 컴포저블 함수 그려지는 동안 단 한번만 할당 연산이 실행될까요?

l2hyunwoo · 2024-05-05T14:42:16Z

app/src/main/java/com/sopt/now/compose/BottomBar.kt

+sealed class BottomNavItem(
+    val route: String,
+    val title: String? = null,
+    val icon: ImageVector? = null
+) {
+    data object Home : BottomNavItem(
+        route = Routes.Home.route,
+        title = "Home",
+        icon = Icons.Default.Home
+    )
+
+    data object Search : BottomNavItem(
+        route = Routes.Search.route,
+        title = "Search",
+        icon = Icons.Default.Search
+    )
+
+    data object MyPage : BottomNavItem(
+        route = Routes.MyPage.route,
+        title = "MyPage",
+        icon = Icons.Default.Person
+    )
+
+}


enum class로도 바꿀 수 있을 것 같습니다.

해당 구조를 sealed로 만드신 이유가 있을까요?

l2hyunwoo · 2024-05-05T14:46:35Z

app/src/main/java/com/sopt/now/compose/screen/ChangePasswordScreen.kt

+    var previousPassword by remember {
+        mutableStateOf("")
+    }
+    var newPassword by remember {
+        mutableStateOf("")
+    }
+    var newPasswordCheck by remember {
+        mutableStateOf("")
+    }


변수명 좋습니다.

l2hyunwoo · 2024-05-05T14:47:08Z

app/src/main/java/com/sopt/now/compose/screen/ChangePasswordScreen.kt

+
+    val navViewModel: NavViewModel =
+        viewModel(viewModelStoreOwner = LocalNavGraphViewModelStoreOwner.current)
+    val authService by lazy { ServicePool.authService }


ViewModel이 선언되어있는데, authService를 뷰단에 끌고온 이유가 있을까요? View와 ViewModel은 어떠한 역할을 맡고 구분되어야 할까요?

l2hyunwoo · 2024-05-05T14:47:59Z

app/src/main/java/com/sopt/now/compose/screen/ChangePasswordScreen.kt

+    fun changePassword(memberId: Int) {
+        val passwordRequest = getPasswordRequestDto()
+        authService.changePassword(memberId, passwordRequest)
+            .enqueue(object : Callback<ResponseDto> {
+                override fun onResponse(
+                    call: Call<ResponseDto>,
+                    response: Response<ResponseDto>,
+                ) {
+                    if (response.isSuccessful) {
+                        Toast.makeText(
+                            context,
+                            "비밀번호 변경 완료! 새 비밀번호로 로그인 하세요",
+                            Toast.LENGTH_SHORT
+                        ).show()
+                        navController.navigate(Routes.Login.route) {
+                            popUpTo(Routes.Login.route) {
+                                inclusive = true
+                            }
+                            launchSingleTop = true


이런 로직들이 뷰에 있으면 뷰모델을 따로 구분해둘 필요가 있을까요?

l2hyunwoo · 2024-05-05T14:49:15Z

app/src/main/java/com/sopt/now/compose/screen/LoginScreen.kt

+) {
+
+    var textId by remember { mutableStateOf("") }
+    var textPw by remember { mutableStateOf("") }


축약어 지양해주세요

l2hyunwoo · 2024-05-05T14:49:35Z

app/src/main/java/com/sopt/now/compose/screen/MyPageScreen.kt

+    val authService by lazy { ServicePool.authService }
+
+
+    Log.d("MyPageScreen", "MyPageScreen start")


이 로그 단 한번만 실행할까요?

t1nm1ksun added 19 commits April 16, 2024 19:40

week2 init commit

74129b2

edit: navigator add

89b12ac

test commit

3e0b41b

Restart

7577c5a

Add: BottomBar visiblity

701b36c

Edit: BottomBar Navigate Success

17a300c

로그인 후 네비게이션바로 마이페이지 이동 시 인자 넘겨주지 않아서 꺼지는 문제 뷰모델 이용해서 해결

Add: LazyColumn add

e02334f

Update HomeScreen.kt

2090347

Merge branch 'feat/week02_compose' into feat/week04_compose

b03535d

Update AndroidManifest.xml

712d1ec

.

3fcf007

add: Add request and response

f8748de

Update SignupScreen.kt

af1baf3

fix: signup with API

81346ab

add: login with API

c6468c3

add: delete stack before login

79dd78e

Screen Foldering

f601c50

add: changePassword Add

15e1426

Update ChangePasswordScreen.kt

c277993

t1nm1ksun requested a review from a team May 3, 2024 09:52

0se0 reviewed May 5, 2024

View reviewed changes

t1nm1ksun added 2 commits May 5, 2024 20:44

fix: reponse combine

5108f8e

ChangePassword Screen BattomBar visible false

ee86bdb

twogarlic reviewed May 5, 2024

View reviewed changes

t1nm1ksun requested review from kez-lab and Sangwook123 May 5, 2024 12:27

l2hyunwoo suggested changes May 5, 2024

View reviewed changes

Update MyProfile.kt

0aa9c07

t1nm1ksun added a commit that referenced this pull request May 24, 2024

[chore] #6 import문 수정

862a2c4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/week04 compose #6

Feat/week04 compose #6

t1nm1ksun commented May 3, 2024

0se0 left a comment

0se0 May 5, 2024 •

edited

Loading

t1nm1ksun May 5, 2024

0se0 May 5, 2024

0se0 May 5, 2024

twogarlic May 5, 2024

l2hyunwoo left a comment

l2hyunwoo May 5, 2024

l2hyunwoo May 5, 2024

l2hyunwoo May 5, 2024

l2hyunwoo May 5, 2024

l2hyunwoo May 5, 2024

l2hyunwoo May 5, 2024

l2hyunwoo May 5, 2024

l2hyunwoo May 5, 2024

l2hyunwoo May 5, 2024

l2hyunwoo May 5, 2024

l2hyunwoo May 5, 2024

		val authService by lazy { ServicePool.authService }


		Log.d("MyPageScreen", "MyPageScreen start")

Feat/week04 compose #6

Are you sure you want to change the base?

Feat/week04 compose #6

Conversation

t1nm1ksun commented May 3, 2024

📌 PR 요약

🌱 작업한 내용

🌱 PR 포인트

0se0 left a comment

Choose a reason for hiding this comment

0se0 May 5, 2024 • edited Loading

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

l2hyunwoo 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

0se0 May 5, 2024 •

edited

Loading