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

[FEAT] WEEK4 - compose 필수과제 #9

Open
wants to merge 3 commits into
base: develop-compose
Choose a base branch
from

Conversation

nagaeng
Copy link
Collaborator

@nagaeng nagaeng commented May 3, 2024

📗 WORK DESCRIPTION

TODO

  • 회원가입 API 서버 통신을 통해 성공 시 토스트 띄우기
  • API 명세서에 맞게 로그인 성공 시 memberId(헤더)를 포함하는 토스트 띄우기
  • 로그인 API 서버 통신을 통해 다른 기기에서도 로그인이 가능하도록 구현
  • 로그인 성공시 POST를 사용해서 memberId를 통한 userinfo 불러와서 마이페이지 구현

📗VIDEO (RUN)

4week-compose-essential.mp4

📗TO REVIEWER

userInfo 불러오는 마이페이지 구현이 미완성입니다 ,,,, 얼른 마치겠습니다 ㅠㅠ

@nagaeng nagaeng changed the title Feat/#6 week4 compose essential [FEAT] WEEK4 - compose 필수과제 May 3, 2024
Copy link
Member

@leeeyubin leeeyubin 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 +97 to +98
data?.let {
continuation.resume(
Copy link
Member

Choose a reason for hiding this comment

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

resume은 어떤 역할을 하는 건가용..? 궁금해서 물어봅니당

Comment on lines +121 to +125

class LoginViewModel : ViewModel() {
private val _loginState = mutableStateOf(LoginState())
fun login(
userId: String,
Copy link
Member

Choose a reason for hiding this comment

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

xml 코리에 달렸던 내용과 동일한데, 뷰모델은 따로 파일화 해주세요!

Comment on lines +110 to +117
@Composable
fun SignUpTextField(
value: String,
onValueChange: (String) -> Unit,
label: String,
hint: String,
isPassword: Boolean = false
) {
Copy link
Member

Choose a reason for hiding this comment

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

TextField 따로 빼준 거 좋네요!! 저도 얼른 리팩 하도록 하겠습니닷

Comment on lines +130 to +132
class SignUpViewModel : ViewModel() {
private val _signUpState = mutableStateOf(SignUpState())
fun signUp(
Copy link
Member

Choose a reason for hiding this comment

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

보통 _signUpStatesignUpState로 나눠서 백킹 프로퍼티를 적용하곤 하는데,
이렇게 사용하면 그 의미가 사라지는 것 같습니다!
이에 대해 공부해보시면 좋을 것 같아요!

Comment on lines +36 to +37
private val viewModel by lazy { LoginViewModel() }

Copy link
Member

Choose a reason for hiding this comment

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

뷰모델을 by lazy로 선언하신 이유는 무엇인가용....?

Comment on lines +37 to +40
LaunchedEffect(userId) {
try {
userInfo = getUserInfo(userId)
} catch (e: Exception) {
Copy link
Member

Choose a reason for hiding this comment

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

단순 궁금증인데 userId를 매개변수로 받아서 LaunchedEffect에 넣어주는 것만으로도 getUserInfo() 함수가 실행이 되나요,,?!

Copy link
Member

@MoonsuKang MoonsuKang left a comment

Choose a reason for hiding this comment

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

날마다 성장하는 이나경 멋있다...

fun login(
userId: String,
userPassword: String,
context: Context
Copy link
Member

Choose a reason for hiding this comment

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

ViewModel에서 Context를 직접 사용하는 것은 권장하지 않습니다!

Comment on lines +144 to +145
context.startActivity(intent)
intent.putExtra("userId", userId)
Copy link
Member

Choose a reason for hiding this comment

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

putExtra를 화면전환 이전에 시키는게 좋지 않을까요?

userPassword: String,
userNickname: String,
userPhone: String,
context: Context
Copy link
Member

Choose a reason for hiding this comment

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

이것도 앞서 말했던 것과 동일합니다~

Log.e("userId", "sign up success for $userId")

// 회원가입 성공 시 로그인 화면으로 이동
val intent = Intent(context, LoginActivity::class.java)
Copy link
Member

Choose a reason for hiding this comment

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

실제 화면 전환은 ViewModel이 아니라 Composable에서 처리할 수 있도록 하는게 어떨까요?

import kotlin.coroutines.resumeWithException
import kotlin.coroutines.suspendCoroutine
@Composable
fun MyPageFragment(context: Context, userId: String) {
Copy link
Member

Choose a reason for hiding this comment

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

컴포저블 함수에 직접 Context를 매개변수로 전달하는 것은 권장하지 않는 것으로 알고있는데 어떤 방법이 있을까요?

}

suspend fun getUserInfo(userId: String): UserInfo {
return suspendCoroutine { continuation ->
Copy link
Member

Choose a reason for hiding this comment

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

suspendCoroutine을 사용하신 이유가 있나요? 그냥 궁금!!

Copy link
Member

@boiledEgg-s boiledEgg-s 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 +56 to +57
var userId by remember { mutableStateOf("") }
var userPassword by remember { mutableStateOf("") }
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 +52 to +55
var userId by remember { mutableStateOf("") }
var userPassword by remember { mutableStateOf("") }
var userNickname by remember { mutableStateOf("") }
var userPhone by remember { mutableStateOf("") }
Copy link
Member

Choose a reason for hiding this comment

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

위에서도 말했듯이 이런 변수들은 뷰모델에서 관리해봐요!!

import android.os.Parcel
import android.os.Parcelable

data class UserInfo(
val id: String,
val password: String,
val nickname: String,
val mbti: String
val phone: String
) : Parcelable {
constructor(parcel: Parcel) : this(
parcel.readString() ?: "",
Copy link
Member

Choose a reason for hiding this comment

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

?: ""와 같은 기능을 하는 orEmpty()라는 메서드가 있답니다~
저도 방금 배움ㅎ

@nagaeng nagaeng self-assigned this May 24, 2024
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.

4 participants