Week2 과제 ... #3
Week2 과제 ... #3
Conversation
tunaunnie
requested review from
chattymin,
jihyunniiii,
t1nm1ksun and
angryPodo
|
|
||
| @Composable | ||
| fun CustomTopAppBarSecond(navController: NavController){ | ||
| TopAppBar( |
There was a problem hiding this comment.
Uncompleted Tasks에 적혀있는 두 TopAppBar 사이의 간격에 대해서는 TopAppBar 자체의 구현 내용을 보시면 좋을 것 같아요. 아래 사진에 보시면 elevation: Dp = AppBarDefaults.TopAppBarElevation 라는 기본값이 있어요.
그리고 AppBarDefaults를 들어가서 보시면 val TopAppBarElevation = 4.dp이 선언되어 있어요. 즉, TopAppBar에는 기본적으로 4.dp만큼의 패딩이 있기 때문에 두 TopAppBar 사이에는 8.dp만큼의 간격이 존재할거에요.
그리고 두번째 가능성은 TopAppBar 자체의 높이가 할당되어있다는 거에요. TopAppBar를 구현한 AppBar를 보시면 AppBarHeight라는 높이만큼 높이를 할당하고 있어요. private val AppBarHeight = 56.dp 그렇기 때문에 의도하신 높이와 다를 수 있어요.
현재 구조상 두개의 TopAppBar를 겹치신다면 TopAppBar의 높이인 56과 위아래 패딩을 합친 8, 이게 두개니까 128.dp의 높이를 가지게 될거에요
| Scaffold( | ||
| topBar = { | ||
| Column { | ||
| CustomTopAppBar(navController = navController) | ||
| CustomTopAppBarSecond(navController = navController) | ||
| } | ||
| }, | ||
| bottomBar = { | ||
| CustomBottomAppBar(navController = navController) | ||
| } |
There was a problem hiding this comment.
이 코드가 Home, Search, My에서 중복되고 있는 것 같아요. 외부로 빼는건 어떨까요?
지금 방법의 문제점은 아래처럼 느껴져요.
- 같은 코드가 여러 화면에서 중복된다.
- 화면 이동을 할 때 bottomBar도 깜빡이며 화면이 재구성된다.
NavHost 자체를 Scaffold로 감싸는 방법도 있을 것 같아요. 그렇게 되면 SignUp과 같은 화면에도 topBar와 bottomBar가 나올거에요. 그 때 조건을 통해 특정 화면일 때에만 나오도록 설정할 수 있어요!
물론 다른 방법이 있다면 그렇게 하는 것도 좋아요! 저는 여러 방법중 하나를 제안드리는거지 절대 정답은 아닙니다 :)
| ) | ||
| composable<LoginScreen> { backStackEntry -> | ||
| val item = backStackEntry.toRoute<LoginScreen>() | ||
| val scope = rememberCoroutineScope() |
There was a problem hiding this comment.
scope변수를 Screen 밖에서 선언한 후 Screen에 매개변수로 넘겨준 이유가 있을까요?
외부에서 scope를 사용하고 있지 않고있기 때문에 Screen 내부에서 선언하는게 좋을 것 같아요.
|
|
||
| @Composable | ||
| fun Greeting3(modifier: Modifier = Modifier) { | ||
| fun MypageScreen( | ||
| navController: NavController, |
There was a problem hiding this comment.
navController를 매개변수로 받아서, 화면 내에서 이동을 하는 방법을 사용하고 있는데 윗부분에서는 navigation과 관련된 함수는 람다를 통해 위로 올려줘서 관리하고 있더라구요. 두가지 방법을 섞어서 사용하신 이유가 있을까요?
우선 저는 navController를 매개변수로 넘겨주는 것을 선호하지 않아요. 그렇게 된다면 화면 내부에 navigation 로직이 존재하게 되고, 추후 수정할 때 하나하나 찾기 귀찮더라구요
그리구 나중에 멀티모듈을 하시게 된다면, 화면 밖으로 빼주는 것의 장점을 한번 더 느끼실 수 있으실 거에요. 물론 나아아아아중에 하시면 되는 개념이고, 심지어 안해보셔도 돼요 :) 걱정하지 않으셔도 됩니당
| @Composable | ||
| fun HomeLazyRow( | ||
| title: String, | ||
| images: List<Int>, | ||
| height: Int, | ||
| width: Int, | ||
| modifier: Modifier = Modifier | ||
| ) { |
There was a problem hiding this comment.
깔끔하게 잘 만드셨네요!! 고민 하시며 만드신게 보여요 :)
하나 수정하면 좋을 것 같은 부분은 변수명을 정하는 부분이에요!
weight와 height라는 변수명은 LazyRow 내부에 있는 Image의 크기를 말하는 것인지, 해당 컴포넌트 자체의 크기를 의미하는 것인지 오해의 소지가 있어요. 그렇기 때문에 조금 더 직관적인 이름을 사용하시면 좋을 것 같아요
| ) { | ||
| Image( | ||
| modifier = Modifier | ||
| .size(70.dp, 70.dp) |
There was a problem hiding this comment.
| .size(70.dp, 70.dp) | |
| .size(70.dp) |
이렇게 해도 같습니당
There was a problem hiding this comment.
동민님 리뷰에 덧붙이자면 이러한 경우를 방지하기위해 Named Arguments를 사용하면 좋습니다!
Text("텍스트") -> Text(text="텍스트") 이렇게요😁
| Text( | ||
| "$sectionDescription", | ||
| color = Color.Gray | ||
| ) | ||
| Text( | ||
| "구매하기 >", | ||
| color = Color.White | ||
| ) |
There was a problem hiding this comment.
named call argument 가 없다면 가독성에서도 아쉬워지고, 추후 수정을 할 때에도 불편할 수 있어요. 붙이는 습관을 들이면 좋을 것 같아요!
| Text( | |
| "$sectionDescription", | |
| color = Color.Gray | |
| ) | |
| Text( | |
| "구매하기 >", | |
| color = Color.White | |
| ) | |
| Text( | |
| text = "$sectionDescription", | |
| color = Color.Gray | |
| ) | |
| Text( | |
| text = "구매하기 >", | |
| color = Color.White | |
| ) |
There was a problem hiding this comment.
아하 named argument에 대한 이야기가 여기 이미 달렸네용
| @Composable | ||
| fun SignUpTextField( | ||
| modifier: Modifier = Modifier, | ||
| onValueChange: (String) -> Unit, | ||
| fieldType: String, //Email 혹은 Password로 전달 예정 | ||
| text: String, | ||
| conditionCheck: Boolean, | ||
| errMessage: String, | ||
| placeholder: String, | ||
| shouldShowPassword: Boolean = false, | ||
| onPasswordVisibilityChange: () -> Unit = {}, | ||
| descriptionText: String = "", | ||
| ){ |
There was a problem hiding this comment.
fieldType에 "Email", "Password"만 들어온다는 제한이 존재한다면 더욱 강력한 제약을 걸면 좋을 것 같아요. Enum으로 만드는 방법도 있을 것 같네요.
그리고 onPasswordVisibilityChange의 경우 Password일 때에만 사용되는 코드 같아요. 그렇다면 이 값을 넣어줬냐, 넣어주지 않았냐로 "Email"인지 "Password"인지를 판단하는 근거로 사용해도 좋을 것 같네요. 물론 명시적이지 않은 방법이여서 가독성을 더 챙기는 과정을 해야할 것 같아요
저라면 textField를 2개를 만들 것 같아요. BaseTextField를 하나 만들어 기본적인 토대를 만들고, 이를 활용하는 TextField와 PasswordTextField 두개로 만들어서 관리한다면 더욱 직관적일 수 있을 것 같아요.
| NavIcon( | ||
| navController = navController, | ||
| route = "home", | ||
| icon = Icons.Filled.Home, | ||
| text = "홈" | ||
| ) | ||
| NavIcon( | ||
| navController = navController, | ||
| route = "search", | ||
| icon = Icons.Filled.Search, | ||
| text = "검색" | ||
| ) | ||
| NavIcon( | ||
| navController = navController, | ||
| route = "profile", | ||
| icon = Icons.Filled.Person, | ||
| text = "MY" | ||
| ) |
There was a problem hiding this comment.
이 세 화면은 route를 String으로 지정하신 이유가 있을까요??
호출하는 부분에서도 String값이라면 옮겨적는 과정에서 실수를 해서 잘못 적을 수도 있고, 일부는 data Class이고 일부는 String이라면 통일성이 없어서 추후 관리하기 헷갈릴 것 같아요
| fun EmailValidCheck(email: String): Boolean { | ||
| var isValid = false | ||
| val inputStr : CharSequence = email | ||
| val pattern = Patterns.EMAIL_ADDRESS | ||
| val matcher = pattern.matcher(inputStr) | ||
| if(matcher.matches()){ | ||
| isValid = true | ||
| } | ||
| return isValid | ||
| } | ||
|
|
||
| fun PasswordValidCheck(password: String): Boolean { | ||
| val pattern = "^(?=.*[a-z])(?=.*[A-Z])(?=.*\\d)|(?=.*[a-z])(?=.*[A-Z])(?=.*[!@#\$%^&*])|(?=.*[a-z])(?=.*\\d)(?=.*[!@#\$%^&*])|(?=.*[A-Z])(?=.*\\d)(?=.*[!@#\$%^&*]).{8,20}\$".toRegex() | ||
| return password.matches(pattern) | ||
|
|
||
| } |
There was a problem hiding this comment.
컴포즈함수가 아니면 함수명은 소문자로 시작해주시는게 좋을거같아요!!
| height: Int, | ||
| width: Int, |
There was a problem hiding this comment.
사용하는 곳 보니까 하나만 사이즈가 다르고 나머지는 똑같아 보이던데 자주 쓰이는 값을 기본값으로 주면 더 편하게 쓸 수 있을거같아요!
(나중에 사용되는 곳이 많아지면 헷갈리더라구요!)
| ) { | ||
| Image( | ||
| modifier = Modifier | ||
| .size(70.dp, 70.dp) |
There was a problem hiding this comment.
동민님 리뷰에 덧붙이자면 이러한 경우를 방지하기위해 Named Arguments를 사용하면 좋습니다!
Text("텍스트") -> Text(text="텍스트") 이렇게요😁
| ) | ||
| Spacer(modifier = Modifier.width(10.dp)) | ||
| Text( | ||
| "${deliveredEmail}님", |
| Text( | ||
| "$sectionDescription", | ||
| color = Color.Gray | ||
| ) | ||
| Text( | ||
| "구매하기 >", | ||
| color = Color.White | ||
| ) |
There was a problem hiding this comment.
아하 named argument에 대한 이야기가 여기 이미 달렸네용
| modifier = modifier.fillMaxWidth() | ||
| ) | ||
| { | ||
| TextField( |
| @@ -0,0 +1,50 @@ | |||
| @file:OptIn(ExperimentalMaterial3Api::class) | |||
There was a problem hiding this comment.
파일 전체단위에 OptIn 어노테이션을 사용하신 이유가 있으신가요?
(저도 잘몰라서 궁금해서요...!)
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| fun MypageScreen( | ||
| navController: NavController, | ||
| userViewModel: UserViewModel = viewModel() | ||
| ) { | ||
|
|
There was a problem hiding this comment.
MypageScreen 함수에서 userViewModel 을 매개변수로 받고 있는데, 이 부분을 @Composable함수 내부에서 선언하는 건 어떨까요? 외부에서 사용되지 않는다면 불필요한 매개변수를 줄일 수 있을 것 같아요!
val userViewModel: UserViewModel = viewModel()
| isValid = true | ||
| } | ||
| return isValid | ||
| } |
There was a problem hiding this comment.
if 문을 사용하여 조건을 확인한 후 값을 설정하는 대신, 조건 자체를 반환하는 방식으로 코드를 간소화 해볼 수 있을 것 같아요!
fun EmailValidCheck(email: String): Boolean {
return Patterns.EMAIL_ADDRESS.matcher(email).matches()
}
There was a problem hiding this comment.
fun EmailValidCheck(email: String): Boolean = Patterns.EMAIL_ADDRESS.matcher(email).matches()
이렇게까지 줄일 수도 있습니다.
| } | ||
| } | ||
| } else { | ||
| /*0개가 아니면 리스트뷰로 보여주기*/ |
There was a problem hiding this comment.
1주차에 제 코드에 달렸던 리뷰인데
| /*0개가 아니면 리스트뷰로 보여주기*/ | |
| //TODO: 리스트뷰로 보여줄 내용 구현 예정 |
를 쓰시면 가독성도 좋고 의미도 확실해 집니다!
| .size(30.dp, 30.dp) | ||
| .clip(CircleShape), | ||
| painter = painterResource(id = R.drawable.btn_kakao), | ||
| contentDescription = "Kakao Logo", |
There was a problem hiding this comment.
그리고 Enum 등을 통해 관리하면 더 가독성이 높아질 것 같아요!
| android:exported="true"> | ||
| <intent-filter> | ||
| <action android:name="android.intent.action.MAIN" /> | ||
| <category android:name="android.intent.category.LAUNCHER" /> | ||
| </intent-filter> | ||
| </activity> | ||
| <activity | ||
| android:name=".HomeActivity" | ||
| android:exported="true"> | ||
| <intent-filter> | ||
| <action android:name="android.intent.action.MAIN" /> | ||
| <category android:name="android.intent.category.LAUNCHER" /> | ||
| </intent-filter> | ||
| </activity> | ||
| <activity | ||
| android:name=".SearchActivity" |
There was a problem hiding this comment.
이번주 과제가 SAA 적용이었죠?
Single Activity가 되려면 말 그대로 액티비티가 하나여야 합니다.
즉, manifest에 등록한 액티비티가 하나여야 해요!
이를 바탕으로 리팩해보시면 좋을 것 같아요!
| @Composable | ||
| fun HomeScreen( | ||
| modifier: Modifier = Modifier, | ||
| navController: NavController, // navController를 넘겨 받아 사용 |
There was a problem hiding this comment.
하위 컴포저블 함수에 NavController를 인자로 받는 것은 권장하지 않는 방법입니다.
필요한 동작들을 함수화해서 함수를 넘겨주세요!
| val images = listOf( | ||
| R.drawable.food_pic1, | ||
| R.drawable.food_pic2, | ||
| R.drawable.food_pic3, | ||
| R.drawable.food_pic4, | ||
| R.drawable.food_pic5 | ||
| ) |
There was a problem hiding this comment.
세미나 때 AAC 뷰모델에 대해 배웠었죠?
이 값은 뷰모델로 옮기면 좋을 것 같아요!!
(왜인지는 세미나 자료 참고)
There was a problem hiding this comment.
추가적으로 persistentListOf에 대해 공부해보시는 걸 추천드려요
| CustomBottomAppBar(navController = navController) | ||
| } | ||
| ) { innerPadding -> | ||
| Column( |
There was a problem hiding this comment.
LazyColumn을 사용하는 것이 성능상으로 더 좋을 것 같아요
(마찬가지로 자세한 내용은 세미나 자료 참고 부탁드립니다.)
| isValid = true | ||
| } | ||
| return isValid | ||
| } |
There was a problem hiding this comment.
fun EmailValidCheck(email: String): Boolean = Patterns.EMAIL_ADDRESS.matcher(email).matches()
이렇게까지 줄일 수도 있습니다.
| var emailText = remember { mutableStateOf("") } | ||
| var passwordText = remember { mutableStateOf("") } | ||
|
|
||
| var shouldShowPassword = remember {mutableStateOf(false)} | ||
| var isEmailValid = remember { mutableStateOf(true) } | ||
| var isPasswordValid = remember { mutableStateOf(true) } |
There was a problem hiding this comment.
해당 값들은 뷰모델에서 관리하는 것이 좋아요!
이번주 과제에 MVVM 아키텍처를 적용하는 것도 포함이었는데요!
나중에라도 꼭 적용해보세요.
| .size(30.dp, 30.dp) | ||
| .clip(CircleShape), | ||
| painter = painterResource(id = R.drawable.btn_kakao), | ||
| contentDescription = "Kakao Logo", |
There was a problem hiding this comment.
그리고 Enum 등을 통해 관리하면 더 가독성이 높아질 것 같아요!
Related issue 🛠
Work Description ✏️
Jetpack Navigation 사용해서 SAA 구조로 화면 전환 적용
로그인 성공 시 접속한 이메일을 ViewModel을 사용하여 전역변수로 관리
Screenshot 📸
SOPT_second_assignment.mp4
Uncompleted Tasks 😅
To Reviewers 📢