-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/#23 login #28
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
Conversation
Walkthrough์ด ํ ๋ฆฌํ์คํธ๋ ๋ก๊ทธ์ธ ๊ธฐ๋ฅ์ ์๋ก์ด ๊ตฌํ์ ์๊ฐํฉ๋๋ค. ํ๋ก์ ํธ์ Hilt ์ข ์์ฑ ์ฃผ์ ์ ํตํฉํ๊ณ , ๋ก๊ทธ์ธ ํ๋ฉด์ ๊ตฌ์กฐ๋ฅผ ๊ฐ์ ํ๋ฉฐ, ์ํ ๊ด๋ฆฌ๋ฅผ ์ํ ์๋ก์ด ์ํคํ ์ฒ๋ฅผ ๋์ ํ์ต๋๋ค. ์ฃผ์ ๋ณ๊ฒฝ ์ฌํญ์ LoginViewModel, LoginState, LoginScreen ๋ฑ์ ์๋ก์ด ์ปดํฌ๋ํธ ์ถ๊ฐ์ ๊ธฐ์กด ๋ค๋น๊ฒ์ด์ ๋ก์ง์ ์ ๋ฐ์ดํธ๋ฅผ ํฌํจํฉ๋๋ค. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? ๐ชง TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
feature/login/src/main/java/com/yapp/feature/login/LoginScreen.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/LoginScreen.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/component/HorizontalDivider.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/component/HorizontalDivider.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/LoginScreen.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/LoginScreen.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/LoginState.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/yapp/app/official/navigation/YappNavHost.kt
Outdated
Show resolved
Hide resolved
@jinukeu |
feature/login/src/main/java/com/yapp/feature/login/component/HorizontalDivider.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/LoginScreen.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/LoginScreen.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/component/HorizontalDivider.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/component/TopTitle.kt
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/component/TopTitle.kt
Outdated
Show resolved
Hide resolved
@jinukeu ๋ฆฌ๋ทฐ ์ฌ์์ฒญ ๋๋ฆฝ๋๋น |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
๋ค์ด๋ฐ ๋ถ๋ถ์ ๊ฐ์ด ๋ง์ถฐ๋ณด๋ฉด ์ข์ ๊ฒ ๊ฐ๋ค์~!
@@ -21,7 +21,7 @@ fun YappNavHost( | |||
modifier = modifier, | |||
) { | |||
loginNavGraph( | |||
signupDestination = { navigator.navigateSignUpScreen() } | |||
onClickSignUp = { navigator.navigateSignUpScreen() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A: navigateToSignUp ๋ค์ด๋ฐ์ด ๋ ์ข์๊ฑฐ๊ฐ์์
is LoginSideEffect.ShowToast -> { | ||
Toast.makeText(context, effect.message, Toast.LENGTH_SHORT).show() | ||
} | ||
is LoginSideEffect.ShowSignUpDialog -> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A: ShowSignUpDialog๋ SideEffect๋ณด๋ค๋ State๋ก ๊ด๋ฆฌํ๋๊ฒ ์ข์๊ฑฐ์์ (๋ค์ PR์์ ํ๋ฒ ํด๋ณด์์ฃ )
feature/login/src/main/java/com/yapp/feature/login/component/LoginInputSection.kt
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/component/LoginSignUpSection.kt
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/component/LoginInputSection.kt
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/component/LoginDivider.kt
Show resolved
Hide resolved
@coderabbitai review |
โ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 21
๐ Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
๐ Files selected for processing (15)
app/src/main/java/com/yapp/app/official/navigation/YappNavHost.kt
(1 hunks)build-logic/src/main/java/com/yapp/HiltAndroid.kt
(1 hunks)core/ui/build.gradle.kts
(0 hunks)feature/login/build.gradle.kts
(1 hunks)feature/login/src/main/java/com/yapp/feature/login/LoginScreen.kt
(1 hunks)feature/login/src/main/java/com/yapp/feature/login/LoginState.kt
(1 hunks)feature/login/src/main/java/com/yapp/feature/login/LoginViewModel.kt
(1 hunks)feature/login/src/main/java/com/yapp/feature/login/component/LoginDivider.kt
(1 hunks)feature/login/src/main/java/com/yapp/feature/login/component/LoginInputSection.kt
(1 hunks)feature/login/src/main/java/com/yapp/feature/login/component/LoginSignUpSection.kt
(1 hunks)feature/login/src/main/java/com/yapp/feature/login/component/TopTitle.kt
(1 hunks)feature/login/src/main/java/com/yapp/feature/login/navigation/LoginNavigation.kt
(1 hunks)feature/login/src/main/res/drawable/illust_login_yappu.xml
(1 hunks)feature/login/src/main/res/values/strings.xml
(1 hunks)gradle/libs.versions.toml
(2 hunks)
๐ค Files with no reviewable changes (1)
- core/ui/build.gradle.kts
๐ Additional comments (8)
feature/login/src/main/res/drawable/illust_login_yappu.xml (1)
1-95
: ์ ๋ฐ์ ์ธ ๋ฒกํฐ ๋๋ก์ด๋ธ ๊ตฌํ์ด ์ ๋์ด์์ต๋๋ค.๋ณต์กํ ์ผ๋ฌ์คํธ๋ ์ด์ ์ ๋ฒกํฐ ๋๋ก์ด๋ธ๋ก ์ ๊ตฌํํ์์ต๋๋ค. ๋ค๋ง, ์์์ ์ธ๊ธํ ์ต์ ํ ์ฌํญ๋ค์ ๊ณ ๋ คํ์ฌ ๊ฐ์ ํ๋ฉด ๋ ์ข์ ๊ฒ ๊ฐ์ต๋๋ค.
build-logic/src/main/java/com/yapp/HiltAndroid.kt (1)
16-16
: Hilt Navigation Compose ์์กด์ฑ ์ถ๊ฐ๊ฐ ์ ์ ํฉ๋๋ค.๋ก๊ทธ์ธ ๊ธฐ๋ฅ์ ๋ค๋น๊ฒ์ด์ ์๊ตฌ์ฌํญ์ ์ํด ํ์ํ ์์กด์ฑ์ด ์ ์ถ๊ฐ๋์์ต๋๋ค.
feature/login/src/main/java/com/yapp/feature/login/LoginState.kt (2)
4-12
: ๐ ๏ธ Refactor suggestion์ํ ํด๋์ค์ ์ผ๊ด์ฑ ์๋ ๋ค์ด๋ฐ์ด ํ์ํฉ๋๋ค.
๋ค์ด๋ฐ ์ผ๊ด์ฑ ๊ฐ์ :
pwFailDescription
โpasswordFailDescription
isPWVisible
โisPasswordVisible
isActivateLoginButton
โisLoginButtonEnabled
๊ธฐ๋ณธ ์ปดํฌ๋ํธ์ ๋ค์ด๋ฐ ๊ท์น์ ๋ฐ๋ฅด๊ธฐ ์ํด:
failDescription
โerrorDescription
data class LoginState( val id: String = "", val password: String = "", - val idFailDescription : String? = null, - val pwFailDescription : String? = null, - val isPWVisible: Boolean = false, + val idErrorDescription: String? = null, + val passwordErrorDescription: String? = null, + val isPasswordVisible: Boolean = false, ){ - val isActivateLoginButton : Boolean = id.isNotEmpty() && password.isNotEmpty() + val isLoginButtonEnabled: Boolean = id.isNotEmpty() && password.isNotEmpty() }Likely invalid or redundant comment.
14-19
: ๐ ๏ธ Refactor suggestionLoginIntent ํด๋์ค์ ์ผ๊ด์ฑ ๊ฐ์ ์ด ํ์ํฉ๋๋ค.
- ๋ค์ด๋ฐ ์ผ๊ด์ฑ:
InputPW
โInputPassword
- ํฌ๋งทํ ์ผ๊ด์ฑ:
- ๊ดํธ ์ฃผ๋ณ ๊ณต๋ฐฑ ์ ๊ฑฐ ํ์
sealed interface LoginIntent { - data class InputID (val id : String) : LoginIntent - data class InputPW ( val pw : String ) : LoginIntent + data class InputId(val id: String) : LoginIntent + data class InputPassword(val password: String) : LoginIntent data object ClickLoginButton : LoginIntent data object ClickSignUpButton : LoginIntent }Likely invalid or redundant comment.
app/src/main/java/com/yapp/app/official/navigation/YappNavHost.kt (1)
24-24
: ๐งน Nitpick (assertive)๋ค๋น๊ฒ์ด์ ํจ์ ๋ค์ด๋ฐ ๊ฐ์ ์ด ํ์ํฉ๋๋ค.
์ด์ ๋ฆฌ๋ทฐ ํผ๋๋ฐฑ์ ๋ฐ๋ผ ๋์์ ๋ ๋ช ํํ๊ฒ ๋ํ๋ด๋ ๋ค์ด๋ฐ์ผ๋ก ๋ณ๊ฒฝํ๋ ๊ฒ์ด ์ข๊ฒ ์ต๋๋ค:
navigateSignUpScreen
โnavigateToSignUp
- onClickSignUp = { navigator.navigateSignUpScreen() } + onClickSignUp = { navigator.navigateToSignUp() }Likely invalid or redundant comment.
feature/login/src/main/java/com/yapp/feature/login/component/LoginInputSection.kt (1)
49-62
: ๐งน Nitpick (assertive)Preview ํจ์๋ฅผ ๊ฐ์ ํ ์ ์์ต๋๋ค.
ํ๋ฆฌ๋ทฐ ํจ์์ ๋ค์ ๊ฐ์ ์ฌํญ์ ์ ์ฉํ๋ฉด ์ข๊ฒ ์ต๋๋ค:
- ๋ถํ์ํ Column ์ ๊ฑฐ
- ์๋ฏธ ์๋ ํ ์คํธ ๋ฐ์ดํฐ ์ฌ์ฉ
- ๋ค์ํ ์ํ ํ ์คํธ (ํ์ฑ/๋นํ์ฑ ๋ฑ)
@Preview(showBackground = true) @Composable private fun LoginScreenPreview() { YappTheme { - Column { LoginInputSection( - "", - "", + email = "[email protected]", + password = "password123", onIDChange = {}, onPWChange = {}, - false, + isActive = true, - {} + onLoginClick = {} ) - } } }Likely invalid or redundant comment.
feature/login/build.gradle.kts (1)
5-5
: Hilt ํ๋ฌ๊ทธ์ธ ์ถ๊ฐ๊ฐ ์ ์ ํฉ๋๋ค.์์กด์ฑ ์ฃผ์ ์ ์ํ Hilt ํ๋ฌ๊ทธ์ธ์ด ์ฌ๋ฐ๋ฅด๊ฒ ์ถ๊ฐ๋์์ต๋๋ค.
gradle/libs.versions.toml (1)
20-21
: Hilt Navigation Compose ์์กด์ฑ์ด ์ ์ ํ๊ฒ ์ถ๊ฐ๋์์ต๋๋ค.๋ฒ์ ๊ณผ ๋ผ์ด๋ธ๋ฌ๋ฆฌ ์ค์ ์ด ์ฌ๋ฐ๋ฅด๊ฒ ๊ตฌ์ฑ๋์ด ์์ต๋๋ค.
Also applies to: 55-56
feature/login/src/main/java/com/yapp/feature/login/component/TopTitle.kt
Show resolved
Hide resolved
@Preview(showBackground = true) | ||
@Composable | ||
private fun LoginScreenPreview() { | ||
YappTheme { | ||
TopTitle() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
๐งน Nitpick (assertive)
ํ๋ฆฌ๋ทฐ ๊ธฐ๋ฅ์ ๋์ฑ ๊ฐ์ ํด๋ณด์ธ์.
ํ๋ฆฌ๋ทฐ๊ฐ ์ ๊ตฌํ๋์ด ์์ต๋๋ค๋ง, ๋ค์ํ ๋๋ฐ์ด์ค ์ค์ ๊ณผ ํ ๋ง์์์ ๋ชจ์ต์ ํ์ธํ ์ ์๋๋ก ๊ฐ์ ํ๋ฉด ์ข์ ๊ฒ ๊ฐ์ต๋๋ค.
๋ค์๊ณผ ๊ฐ์ด ์์ ํด๋ณด์ธ์:
@Preview(showBackground = true)
+@Preview(showBackground = true, uiMode = Configuration.UI_MODE_NIGHT_YES)
+@Preview(showBackground = true, device = Devices.TABLET)
@Composable
private fun LoginScreenPreview() {
YappTheme {
TopTitle()
}
}
ํ์ํ import๋ฌธ๋ ์ถ๊ฐํด์ฃผ์ธ์:
import android.content.res.Configuration
import androidx.compose.ui.tooling.preview.Devices
feature/login/src/main/java/com/yapp/feature/login/component/TopTitle.kt
Show resolved
Hide resolved
id : String, | ||
pw : String, | ||
onIDChange: (String) -> Unit, | ||
onPWChange: (String) -> Unit, | ||
isActive : Boolean, | ||
clickButton : () -> Unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
๐งน Nitpick (assertive)
ํ๋ผ๋ฏธํฐ ๋ค์ด๋ฐ์ ์ผ๊ด์ฑ ์๊ฒ ์์ ํด์ฃผ์ธ์.
ํ์ฌ ํ๋ผ๋ฏธํฐ ๋ค์ด๋ฐ์ด ์ผ๊ด์ฑ์ด ์์ต๋๋ค:
id
,pw
๋ ๋ ๋ช ํํ ์ด๋ฆ์ด ์ข์ต๋๋คclickButton
์ ๋ค๋ฅธ ์ฝ๋ฐฑ ๋ค์ด๋ฐ๊ณผ ์ผ์น์ํค๋ ๊ฒ์ด ์ข์ต๋๋ค
- id : String,
- pw : String,
+ email: String,
+ password: String,
onIDChange: (String) -> Unit,
onPWChange: (String) -> Unit,
isActive : Boolean,
- clickButton : () -> Unit
+ onLoginClick: () -> Unit
๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
id : String, | |
pw : String, | |
onIDChange: (String) -> Unit, | |
onPWChange: (String) -> Unit, | |
isActive : Boolean, | |
clickButton : () -> Unit | |
email: String, | |
password: String, | |
onIDChange: (String) -> Unit, | |
onPWChange: (String) -> Unit, | |
isActive : Boolean, | |
onLoginClick: () -> Unit |
onPWChange : (String) -> Unit, | ||
) { | ||
Column { | ||
Spacer(Modifier.height(72.dp)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
๐งน Nitpick (assertive)
ํ๋์ฝ๋ฉ๋ dimension ๊ฐ๋ค์ ๋ฆฌ์์ค๋ก ์ถ์ถํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
์ฌ๋ฌ ๊ณณ์์ ์ฌ์ฉ๋๋ spacing ๊ฐ๋ค(72.dp, 24.dp, 32.dp)์ dimens.xml ํ์ผ๋ก ์ถ์ถํ์ฌ ์ฌ์ฌ์ฉ์ฑ์ ๋์ด๊ณ ์ผ๊ด์ฑ์ ์ ์งํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
Also applies to: 77-77, 79-79
internal fun LoginRouteScreen( | ||
onClickSignUP: (String) -> Unit, | ||
modifier: Modifier = Modifier, | ||
viewModel: LoginViewModel = hiltViewModel() | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
๐งน Nitpick (assertive)
ํ๋ผ๋ฏธํฐ ๋ค์ด๋ฐ ์ผ๊ด์ฑ ๊ฐ์ ์ด ํ์ํฉ๋๋ค.
onClickSignUP
์ 'UP' ๋ถ๋ถ์ด ์ผ๋ฐ์ ์ธ ๋ค์ด๋ฐ ์ปจ๋ฒค์
๊ณผ ๋ง์ง ์์ต๋๋ค. onClickSignUp
์ผ๋ก ์์ ํ๋ ๊ฒ์ด ์ข๊ฒ ์ต๋๋ค.
- onClickSignUP: (String) -> Unit,
+ onClickSignUp: (String) -> Unit,
๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
internal fun LoginRouteScreen( | |
onClickSignUP: (String) -> Unit, | |
modifier: Modifier = Modifier, | |
viewModel: LoginViewModel = hiltViewModel() | |
) { | |
internal fun LoginRouteScreen( | |
onClickSignUp: (String) -> Unit, | |
modifier: Modifier = Modifier, | |
viewModel: LoginViewModel = hiltViewModel() | |
) { |
๐ก Issue
๐ฑ Key changes
โ To Reviewers
screen ๋จ์๋ก ๊ณ ๋ คํด์ state ์ ์ธํ์ต๋๋ค !
sealed class๋ก ํ๋์ State๋ฅผ ๋ง๋ค๊ณ ๊ตฌํ vs
ํ๋์ State data class๋ก ๊ด๋ฆฌํ๊ธฐ
( data class ๋ก ๊ฐ๋ฐ )
๋ด์ฉ ์ ๋ฆฌ
๐ธ ์คํฌ๋ฆฐ์ท
Summary by CodeRabbit
์๋ก์ด ๊ธฐ๋ฅ
๋ฒ๊ทธ ์์
UI ๋ณ๊ฒฝ
์ข ์์ฑ ์ ๋ฐ์ดํธ