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

[Feature/#6] : 로그인, 회원가입 페이지 구현 #11

Open
wants to merge 62 commits into
base: develop
Choose a base branch
from

Conversation

youjin09222
Copy link

@youjin09222 youjin09222 commented Jan 2, 2025

✅ 𝗖𝗵𝗲𝗰𝗸-𝗟𝗶𝘀𝘁

  • merge할 브랜치의 위치를 확인해 주세요.(main❌/develop⭕)
  • 리뷰가 필요한 경우 리뷰어를 지정해 주세요.
  • 리뷰는 PR이 올라오면 최대한 빠르게 진행합니다.
  • P1 단계의 리뷰는 빠르게 확인 후 반영합니다.
  • Approve된 PR은 assigner가 머지하고, 수정 요청이 온 경우 수정 후 다시 push를 합니다.

📌 𝗜𝘀𝘀𝘂𝗲𝘀

📎 𝗪𝗼𝗿𝗸 𝗗𝗲𝘀𝗰𝗿𝗶𝗽𝘁𝗶𝗼𝗻

  • 로그인 UI
  • 프로필 설정 UI
  • 그룹 초대 코드 입력 UI

📷 𝗦𝗰𝗿𝗲𝗲𝗻𝘀𝗵𝗼𝘁

KakaoTalk_20250103_001410913.mp4

💬 𝗧𝗼 𝗥𝗲𝘃𝗶𝗲𝘄𝗲𝗿𝘀

ktlint 포멧 검사가 잘 안돼서 빠른 시일 내에 해결하겠습니다..!

@youjin09222 youjin09222 added 💟 [UI] UI 작업 🐰 유진 뉴진스 labels Jan 2, 2025
@youjin09222 youjin09222 self-assigned this Jan 2, 2025
@youjin09222 youjin09222 requested a review from a team as a code owner January 2, 2025 14:55
Copy link
Contributor

@gaeulzzang gaeulzzang left a comment

Choose a reason for hiding this comment

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

navGraph 부분만 수정하면 좋을 것 같아욥~!! 배워가는게 너무 많네욤... 🫶

import com.sopt.presentation.R

@Composable
internal fun AuthButton(
Copy link
Contributor

Choose a reason for hiding this comment

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

P3: internal 키워드를 사용하신 이유가 뭔가요!!??

Copy link
Author

@youjin09222 youjin09222 Jan 2, 2025

Choose a reason for hiding this comment

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

같은 모듈 내에서만 접근 가능하도록 제한해서 캡슐화를 강화하고,
불필요하게 외부 모듈에서 사용될 가능성을 줄이고자 했습니다! 혹시 더 좋은 방법이 있을까요!!

Copy link
Contributor

Choose a reason for hiding this comment

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

중복되는 컴포넌트는 core/designsystem에 넣는게 좋을 것 같아서 이 모듈에 넣으면 internal 키워드는 제거해야될 것 같습니답

Copy link
Author

Choose a reason for hiding this comment

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

넵 수정하겠습니다~!!

) {
Text(
text = exampleText,
style = NoostakTheme.typography.b5Regular.copy(color = Gray800),
Copy link
Contributor

Choose a reason for hiding this comment

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

P3: color랑 font를 한번에 적용할 수 있군요..!!

Copy link
Author

Choose a reason for hiding this comment

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

맞아요! 그런데 가독성 부분에서는 따로 쓰는게 좋을 것 같아 고민되네요..!

Copy link
Contributor

Choose a reason for hiding this comment

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

nope 개성 지켜.

Comment on lines 10 to 12
fun navigateToSignup() {
emitSideEffect(LoginSideEffect.NavigateSignUp("authId"))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fun navigateToSignup() {
emitSideEffect(LoginSideEffect.NavigateSignUp("authId"))
}
fun navigateToSignup(authId: String) {
emitSideEffect(LoginSideEffect.NavigateSignUp(authId))
}

P1: 매개변수를 줘야 나중에 값 전달할 때 편할 것 같습니다

Comment on lines 21 to 22
navigateHome: () -> Unit,
navigateSignUp: (String) -> Unit
Copy link
Contributor

Choose a reason for hiding this comment

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

P1: navGraph 안에 화면 이동 로직을 넣어주기 보다는 LoginRoute 안에서 구현하는게 좋을 것 같습니답. navGraph 안에 매개변수가 많아지면 MainScreen에 navGraph 등록할 때 힘들거든요 😢

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다~!

Comment on lines 29 to 30
containerColor: Color = Blue600,
contentColor: Color = White,
Copy link
Contributor

Choose a reason for hiding this comment

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

P3: 사실 잘 몰라서 그러는데 Blue600과 NoostakTheme.colors.blue600의 차이가 뭘까요...

Copy link
Author

@youjin09222 youjin09222 Jan 2, 2025

Choose a reason for hiding this comment

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

간단하게 쓰려고 Blue600 쓴건데 이렇게 하는 경우는 색상을 테마와 분리하여 사용하는 것이기 때문에
하드코딩한 것과 유사하다고 하네요! 후딱 바꾸겠습니다!

NoostakTheme.colors.blue600 : NoostakColors 클래스의 값을 동적으로 참조
Blue600 : 정적인 상수 값 직접 참조

AuthButton(
padding = PaddingValues(vertical = 15.dp),
onClick = onSignUpClick,
isEnabled = isNextEnabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

P2: isNextEnabled 변수를 새로 만들기 보다는 name.isBlank로 둬도 될 것 같아욥

Copy link
Author

Choose a reason for hiding this comment

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

넵 수정하겠습니다~!

}

fun NavGraphBuilder.signUpNavGraph(
navigateToCheckInvite: (String) -> Unit
Copy link
Contributor

Choose a reason for hiding this comment

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

P1: 여기도 수정 부탁드려요!!

Column(
modifier = Modifier
.fillMaxSize()
.padding(16.dp),
Copy link
Contributor

Choose a reason for hiding this comment

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

P1: dimen.xml에 있는 padding 값으로 넣어주세욥~!!

Comment on lines 10 to 19
abstract class BaseViewModel<T> : ViewModel() {
private val _sideEffects = MutableSharedFlow<T>()
val sideEffects: SharedFlow<T> get() = _sideEffects.asSharedFlow()

protected fun emitSideEffect(sideEffect: T) {
viewModelScope.launch {
_sideEffects.emit(sideEffect)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

P3: 야무져요 엄마 ❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

P3: 아 맞다 core/util에 BaseViewModel 넣어놨어욥

Copy link
Author

Choose a reason for hiding this comment

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

어머 고맙습니다ㅏㅏ 그럼 제가 추가한건 삭제해놓을게요~!

<string name="btn_submit">확인</string>

<!-- 로그인 -->
<string name="tv_login_description">SNS 계정으로 빠르게 시작하기</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

P3: xml을 안쓰지만 네이밍이 xml인...ㅠㅠ 저도 사실 이렇게 했는데 다른 좋은 방법 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

오 그렇네요..? 저도 한번 고민해봐야겠습니다🤔

Copy link
Member

Choose a reason for hiding this comment

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

P3 : 저는 걍 text_로 했어요

Copy link
Member

@Eonji-sw Eonji-sw left a comment

Choose a reason for hiding this comment

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

잘하셨네요! 저랑 겹치는 부분이 정말 많아서 음... 파트 분배를 더 잘할 수 있지 않았을까 아니면 초반에 컴포넌트를 나눠가져서 개발을 진행했어야 했나 라는 생각이 정말 많이 드네요 제가 마지막으로 PR 올릴 것 같은데 그때 잘 맞춰서 컴포넌트 재사용할 수 있게 해야겠네요.... color 호출 부분들과 dimens 값 활용만 수정하면 더 좋을 것 같네요

package com.sopt.presentation.auth.signup
data class SignUpState(
val name: String = "",
val profileImage: String = "basic",
Copy link
Member

Choose a reason for hiding this comment

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

P2 : nullable 하지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

수정하겠습니다!!

<string name="btn_submit">확인</string>

<!-- 로그인 -->
<string name="tv_login_description">SNS 계정으로 빠르게 시작하기</string>
Copy link
Member

Choose a reason for hiding this comment

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

P3 : 저는 걍 text_로 했어요

Copy link
Member

@Eonji-sw Eonji-sw left a comment

Choose a reason for hiding this comment

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

아 그리고 갤러리 로직은 아직 작성하지 않으신건가요? 생각해봤는데 제 부분에도 갤러리 접근이 있어서 맨 처음 앱 들어오면 갤러리 권한 받는게 좋을 것 같아요! 다들 어떻게 생각하시나용? @gaeulzzang

Eonji-sw and others added 30 commits January 8, 2025 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐰 유진 뉴진스 💟 [UI] UI 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UI] : 회원가입 UI
4 participants