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

Open
wants to merge 13 commits into
base: kimhm0728
Choose a base branch
from

Conversation

kimhm0728
Copy link

안녕하세요 에디!!!!!! 오랜만이네요 반가와요 ㅎ
Compose가 처음이라서 Composable 함수를 어디에 위치시켜야 할지, 어디까지 함수를 분리해야 할지 감이 잘 안 잡히네요.

step1, step2 변경 사항을 따로 보고 싶다면 아래 링크에서 확인하실 수 있어요~

아래는 step2 UI 영상입니다.
최대한 Figma에 있는 디자인과 동일하게 구현하려고 노력해봤어요!

Screen_Recording_20241008_164625_Signup.mp4

이번 미션 잘 부탁드립니다 🫡

@kimhm0728 kimhm0728 self-assigned this Oct 8, 2024
Copy link
Member

@junjange junjange left a comment

Choose a reason for hiding this comment

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

안녕하세요 올리브 !! 🫡
구현을 정말 잘 해주셨네요! 항상 올리브의 PR을 보면서 많은 것을 배우고 있습니다.
저도 올리브처럼 PR을 잘 작성하고 싶어요! 하핳
질문 주신 내용을 고려하여, 제가 생각하는 도움?이 될 만한 부분에 코멘트를 남겼습니다.
한번 고민해보시고 답변 주시면 감사하겠습니다!

Comment on lines +52 to +55
SignUpInput(titleId = R.string.username_input)
SignUpInput(titleId = R.string.email_input)
SignUpInput(titleId = R.string.password_input, isPassword = true)
SignUpInput(titleId = R.string.password_confirm_input, isPassword = true)
Copy link
Member

Choose a reason for hiding this comment

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

만약에 Column이 아닌 다른 Composable를 사용하지 않고 SignUpInput()이라는 Composable이 여러개 필요한 UI라면 어떤식으로 구현할 것 같으신가요?!

SignUpInput(titleId = R.string.username_input)
SignUpInput(titleId = R.string.username_input)
SignUpInput(titleId = R.string.username_input)
SignUpInput(titleId = R.string.username_input)
SignUpInput(titleId = R.string.username_input)
SignUpInput(titleId = R.string.username_input)
SignUpInput(titleId = R.string.username_input)

Comment on lines +23 to +26
@Composable
fun Space(dp: Int) {
Spacer(modifier = Modifier.height(dp.dp))
}
Copy link
Member

@junjange junjange Oct 8, 2024

Choose a reason for hiding this comment

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

해당 Composable의 파라미터 타입에 대해 조금 더 고민해보시면 좋을 것 같습니다! 네이밍의 문제일 수도 있지만, 재사용성이 얼마나 좋은지도 한번 고민해보시면 좋겠습니다.

Comment on lines +15 to +19
Surface(
modifier = Modifier.fillMaxSize(),
) {
content()
}
Copy link
Member

Choose a reason for hiding this comment

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

Surface를 사용하신 이유가 있으신가요?!

Comment on lines +111 to +133
@Composable
private fun SignUpButton() {
Button(
onClick = {
// TODO
},
contentPadding = PaddingValues(0.dp),
colors = ButtonDefaults.buttonColors(
containerColor = Blue50,
),
modifier =
Modifier
.fillMaxWidth()
.padding(horizontal = 32.dp),
) {
Text(
text = stringResource(id = R.string.sign_up_button),
fontSize = 14.sp,
fontWeight = FontWeight.W500,
modifier = Modifier.padding(vertical = 15.dp)
)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

여러 곳에서 다양한 크기의 SignUpButton을 사용하고자 할 때, 해당 Composable을 어떻게 개선할 수 있을까요?!

Comment on lines +1 to +15
# android-signup
## 1단계 - 컴포즈 기초
### 기능 요구사항
- [x] text() 테스트를 성공하도록 수정한다.
- [x] column() 테스트를 성공하도록 수정한다.
- [x] button() 테스트를 성공하도록 수정한다.

## 2단계 - 회원가입(뷰)
### 기능 요구사항
- [x] 디자인 시안을 참고하여 회원가입 뷰를 구현한다.

### 프로그래밍 요구사항
- [x] ViewModel, Hilt 등은 활용하지 않는다.
- [x] 컴포저블 함수가 너무 많은 일을 하지 않도록 분리한다.
- [x] Material3 Button, TextField를 활용한다.
Copy link
Member

Choose a reason for hiding this comment

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

굿굿

Comment on lines +28 to +31
class LayoutBasicsTest {
@get:Rule
val composeTestRule = createComposeRule()

Copy link
Member

Choose a reason for hiding this comment

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

Android ViewCompose로 UI 테스트를 진행해봤을 때 어떤 차이점이 있었나요?

단순 질문입니다,,하핳

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