Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[벼리] 회원가입 미션 1, 2단계 제출합니다. #16

Open
wants to merge 10 commits into
base: gaeun5744
Choose a base branch
from

Conversation

gaeun5744
Copy link

안녕하세요 올리브~ 오랜만이에요~
이번 미션 잘 부탁드립니다 😊

전달 사항

  • SingleLineTextField

    • 디자이너마다 다르지만,, 저런 textField는 고정된 2~3가지의 종류의 디자인으로 컴포넌트화 시키는 경우가 많았어서 modifier는 변수로 넘기지 않도록 했습니다!!
  • SubmitButton

    • 버튼도 위와 마찬가지의 이유로 구성했습니다~
  • data class

    • name, email 등을 하나로 감싸는 SignUpInfo data class를 만들까 하다가,,, viewModel과 같은 객체와 상호작용할 때 이런 객체화가 의미있는 것 같아서 우선은 미뤘습니당
Screen_recording_20241011_152051.webm

@gaeun5744 gaeun5744 self-assigned this Oct 11, 2024
@gaeun5744 gaeun5744 changed the base branch from main to gaeun5744 October 11, 2024 06:38
Copy link

@kimhm0728 kimhm0728 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 벼리~!
워낙 잘 구현해주셔서, 궁금한 부분 위주로 코멘트 달아보았어요.
반영할 부분이 없다면 코멘트에 답변만 달고 다시 리뷰 요청 해주셔도 좋습니다!

Comment on lines +21 to +35
Button(
content = {
Text(text, fontSize = 14.sp)
},
shape = RoundedCornerShape(100.dp),
onClick = onClick,
contentPadding = PaddingValues(15.dp),
modifier = Modifier.fillMaxWidth(),
colors = ButtonColors(
containerColor = Blue50,
contentColor = Color.White,
disabledContentColor = Gray20,
disabledContainerColor = Gray50
)
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

벼리는 함수의 마지막 파라미터로 람다를 전달해야 하는 경우, 람다를 괄호 밖으로 빼지 않고 파라미터명을 명시해서 전달하는 편인가요?

    Button(
        // ...
    ) {
        Text(text, fontSize = 14.sp)
    }

저는 위 코드처럼 content는 밖으로 빼내는 편이에요. Button 안에 Text가 존재한다는 계층적 구조가 잘 표현되어있기도 하고요.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스크린샷 2024-10-12 오전 2 17 34 특히 Compose의 content 매개변수는 더더욱 람다 밖으로 빼는 것이 일반적인 것 같아요. Compose의 내부 구현 뜯어봐도 유사한 형태고요~! 이에 대해 벼리의 생각이 궁금합니다!

Column(
modifier = Modifier.testTag("이름")
) {
Text(text = "$text", color = Color.Gray)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Text(text = "$text", color = Color.Gray)
Text(text = text, color = Color.Gray)

사소하지만 text 파라미터 타입 자체가 String이라 위처럼 바꿀 수 있겠네요~!

@Composable
fun TextViewTest(text: String) {
Text(
text = "$text",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 마찬가지입니다

import nextstep.signup.ui.theme.Gray50

@Composable
fun SingleLineTextField(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기능이 추가되면 한줄만 작성할 수 있는 TextField가 여러개가 생길 수 있을 것 같아요. 그래서 SingleLineTextField라는 네이밍은 조금 광범위하다는 느낌이 드는데 조금 더 구체화하는 건 어떨까요??

import androidx.compose.ui.unit.sp

@Composable
fun TitleText(text: String) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

회원가입 화면에 사용되는 모든 컴포넌트를 public 컴포넌트로 분리해주셨는데, 컴포넌트를 분리하는 벼리만의 기준이 있을까요? 있다면 어떤 건지도 궁금해요!

TitleText를 컴포넌트로 분리하는게 기존 xml에서 font styles 지정하는 것과 유사하게 느껴지네요. xml styles이라고 생각하면 TitleText도 컴포넌트로 분리하는게 자연스러운 것 같기도 하고?! Compose가 처음이다보니 컴포넌트 분리가 어려워요. ㅋㅋ

Comment on lines +24 to +28
text: String,
onTextChange: (String) -> Unit,
hint: String,
keyBoardType: KeyboardType = KeyboardType.Text,
visualTransformation: VisualTransformation = VisualTransformation.None,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

디자이너마다 다르지만,, 저런 textField는 고정된 2~3가지의 종류의 디자인으로 컴포넌트화 시키는 경우가 많았어서 modifier는 변수로 넘기지 않도록 했습니다!!

그런데 저라면 Modifier 변수를 넘기도록 구현했을 것 같아요.

혹시 SurfaceScaffold의 차이를 아시나요?
ScaffoldTopBar, BottomBar 등을 추가할 수 있는 레이아웃이라고 해요. 그런데 Scaffold의 content 람다로 PaddingValues를 받아올 수 있는데요. 이 PaddingValuesTopBarBottomBar이 추가되었을 때, 추가된 TopBarBottomBar만큼의 padding 값이에요.
content의 최상위 컴포넌트들은 꼭 padding 값을 설정해주어야 한다고 알고 있어요. 컴포넌트들에 padding을 주지 않으면 TopBar 등에 가려져서 컴포넌트가 보이지 않을 수도 있다고 합니다.

이처럼 고정된 컴포넌트라도 padding과 같은 옵션을 줘야할 상황이 올 수 있을 것 같은데, 기본 Modifier를 default parameter로 넘겨주면서 컴포넌트의 자유도를 조금 높이는 건 어떨까요??

참고 글

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그런데 Compose를 잘 알지 못하다보니 이런 경우가 TextField, Button 컴포넌트들에도 적용되는지는 모르겠어요.
저는 위 이유가 아니더라도 자주 사용되는 컴포넌트라면 더 자유롭게 속성을 변경할 수 있어야한다는 입장이긴 합니다. MainActivity의 Spacer 대신 Modifier의 padding으로 넘겨줄 수 있기도 하고요.

import nextstep.signup.ui.theme.Gray50

@Composable
fun SubmitButton(onClick: () -> Unit, text: String) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이전 리뷰와 마찬가지로 저는 람다 파라미터를 가장 마지막 파라미터로 두는게 좋아요~

SubmitButton("벼리바보") {
      // ...
}

물론 onClick 람다라 성격이 조금 다르지만, 사용하는 쪽에서 이렇게 작성하면 더 잘 읽힌다고 생각해서요. 이건 개인 스타일 차이일 것 같네요. 하지만 벼리의 의견이 궁금합니다~!

Comment on lines +4 to +9
<string name="sign_up_title">Welcome to Compose 🚀</string>
<string name="sign_up_input_user_name">Username</string>
<string name="sign_up_input_user_email">Email</string>
<string name="sign_up_input_user_password">Password</string>
<string name="sign_up_input_user_password_confirm">Password Confirm</string>
<string name="sign_up_submit_btn">Sign Up</string>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants