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단계 제출합니다. #10

Open
wants to merge 14 commits into
base: aprilgom
Choose a base branch
from

Conversation

aprilgom
Copy link

안녕하세요 해음
이번엔 리뷰어로 만나게 되었네요 ✋
리뷰잘부탁드려요

@aprilgom aprilgom self-assigned this Oct 10, 2024
Copy link

@haeum808 haeum808 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 +1 to +2
[*.{kt,kts}]
ktlint_function_naming_ignore_when_annotated_with=Composable

Choose a reason for hiding this comment

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

ktlint 의존성 추가는 안보이는데 추가하신건가요?

Text(
text = text,
fontSize = 14.sp,
modifier = modifier.padding(15.dp),

Choose a reason for hiding this comment

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

Button에 modifier를 주지 않은이유가 궁금합니다!
ConfirmButton이 Modifier를 주는데 Text가 가져가서요!

label: String,
text: MutableState<String>,
onValueChange: (String) -> Unit = {},
) = SignUpTextField(
Copy link

@haeum808 haeum808 Oct 11, 2024

Choose a reason for hiding this comment

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

keyboardOptions도 활용해보는건 어떨까요!

import nextstep.signup.ui.theme.SignupTheme

@Composable
fun SignUpPasswordTextField(

Choose a reason for hiding this comment

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

로그인할때 중복 사용을 대비해서 SignUpPasswordTextField -> PasswordTextField 어떤가요?

visualTransformation: VisualTransformation = VisualTransformation.None,
modifier: Modifier = Modifier,
) {
TextField(

Choose a reason for hiding this comment

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

여기도 keyboardOptions도 활용해보는건 어떨까요!

onBackground = Color(0xFF1C1B1F),
onSurface = Color(0xFF1C1B1F),
*/
primary = Blue50,

Choose a reason for hiding this comment

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

꼼꼼하시군요!

<string name="welcome">Welcome to Compose \uD83D\uDE80</string>

Choose a reason for hiding this comment

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

문자열 분리 좋네요!

isPasswordSame

Text(
modifier = Modifier.padding(bottom = 42.dp),

Choose a reason for hiding this comment

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

Spacer로 간격 주는건 어떤가요?

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