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/jaino/#38 #39

Merged
merged 38 commits into from
Oct 27, 2023
Merged

Feature/jaino/#38 #39

merged 38 commits into from
Oct 27, 2023

Conversation

jeongjaino
Copy link
Member

1. 📄 관련된 이슈 및 소개

#38 운영진 인증 디알로그 구현

2. 🔥변경된 점

  1. 운영진 검증 디알로그 구현
  2. 첫 운영진 페이지 입장 시 운영진 검증,
  3. 등록되지 않은 운영진인 경우 디알로그로 인증
  • 인증 시 코드 검증과 운영진 등록을 한번에 되도록 구현

3. 📸 스크린샷(선택)

image image

4. 💡알게된 혹은 궁금한 사항들

status vs state,
verification validation
mange - noun : managenet

@jeongjaino jeongjaino added 🚀진호🚀 ESTP 정진호 23세 🌱기능🌱 새로운 기능 두두둥장! 🎨유아이🎨 피그마, 엑세멜, 컴포즈 유아이/유엑스 작업 🔥리뷰 기다리는 중🔥 PR을 올리고 코드리뷰를 기다리고 있는 상태입니다. labels Oct 23, 2023
@jeongjaino jeongjaino self-assigned this Oct 23, 2023
@jeongjaino
Copy link
Member Author

management를 알아차리기 전과 후에서 manage 와 management 두 개의 단어가 같이 있어요!

나중에 이슈파서 작업할꺼라, 일부로 남겨놨습니다. !!

@jeongjaino jeongjaino closed this Oct 23, 2023
@jeongjaino jeongjaino reopened this Oct 23, 2023
@jeongjaino jeongjaino closed this Oct 23, 2023
@jeongjaino jeongjaino reopened this Oct 23, 2023
Copy link
Member

@tgyuuAn tgyuuAn 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 6 to 12
fun Context.showToast(message: String) {
Toast.makeText(this, message, Toast.LENGTH_SHORT).show()
}

fun Context.showLongToast(message: String) {
Toast.makeText(this, message, Toast.LENGTH_LONG).show()
}
Copy link
Member

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.

Activity 와 Context 의 확장 함수 중 어떤 게 더 적합할까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

Activity에서만 호출되는 익스텐션이라 Acitivty 쪽으로 수정하는게 맞는 것 같네요! 수정하겠습니다

Comment on lines +28 to +32
fun ManageCodeDialog(
viewModel: ManagementCodeViewModel = hiltViewModel(),
onDismissRequest: () -> Unit,
showToast: (Throwable) -> Unit,
) {
Copy link
Member

Choose a reason for hiding this comment

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

ManageCodeDialog -> VerifyManagerCodeDialog
는 어떨까욥?!

Copy link
Member Author

Choose a reason for hiding this comment

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

40번 풀 리퀘스트에서 ManagementCodeDialog로 변경되었습니다.

생각해보니까 검증이라는 단어가 들어가면 더 좋을 것 같아요.

제가 짜놓고도 사실 좀 애매한 네이밍이라, 하하

ManagementCodeValidationDialog 는 어떤가요??

Copy link
Member

Choose a reason for hiding this comment

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

넵 둘 중에 아무거나 하셔도 상관없습니다. 더 마음에 드는 걸로 하시면 좋을 것 같아요...!

import kotlinx.coroutines.launch

@HiltViewModel
class ManagementCodeViewModel @Inject constructor(
Copy link
Member

Choose a reason for hiding this comment

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

ManagementCodeVideModel 보다 혹시 더 좋은 네이밍이 있을까요...?

Copy link
Member Author

Choose a reason for hiding this comment

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

ManagementDialogViewModel 도 괜찮을 것 같고

위 리뷰랑 연관 시켜서

ManagementDialogValidationViewModel도 괜찮을 것 같아요

Copy link
Member

Choose a reason for hiding this comment

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

만약에 하게 된다면 위에서 정한 다이얼로그 네임과 연계되는 이름으로 하는게 더 알아보기 편할 것 같아요.

어차피 해당 다이얼로그에만 해당하는 뷰모델이니.

Copy link
Member

@SeongHoonC SeongHoonC 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 6 to 12
fun Context.showToast(message: String) {
Toast.makeText(this, message, Toast.LENGTH_SHORT).show()
}

fun Context.showLongToast(message: String) {
Toast.makeText(this, message, Toast.LENGTH_LONG).show()
}
Copy link
Member

Choose a reason for hiding this comment

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

Activity 와 Context 의 확장 함수 중 어떤 게 더 적합할까요?

@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun ManageCodeDialog(
viewModel: ManagementCodeViewModel = hiltViewModel(),
Copy link
Member

Choose a reason for hiding this comment

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

컴포즈 보니깐 hiltViewModel(), koinViewModel() 이 있네요..신기

Copy link
Member Author

Choose a reason for hiding this comment

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

implementation("androidx.hilt:hilt-navigation-compose:1.0.0") 종속성 안에 포함되어있는데,

Compose가 보통 SAA 구조에서 Composable 함수끼리 Navigation을 하다보니, 따로 Activity에서 ViewModel 생성해서 주입하는게 비용이 커서 만들어진 라이브러리인 것 같아요.

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 +58 to +59
data object Init : ManagementCodeUiState()
data object Success : ManagementCodeUiState()
Copy link
Member

Choose a reason for hiding this comment

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

오 data object..

Copy link
Member Author

Choose a reason for hiding this comment

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

pknujsp의 1.9.0 kotlin posting을 확인해주세요 !

Comment on lines +23 to +24
private val _manageCode: MutableStateFlow<String> = MutableStateFlow("")
val manageCode: StateFlow<String> get() = _manageCode
Copy link
Member

Choose a reason for hiding this comment

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

혹시 컴포즈에는 two-way 바인딩 같은게 없을까용

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 ViewModel을 참조하고 있는 Composable 함수를 보시면,

해당 StateFlow를 CollectAsState라는 익스텐션으로 받고 있습니다. 해당 함수는 StateFlow에서 수신된 값을 Compose State로 바꿔주는 확장함수인데, 이를 사용하면 Compose Component에 상태를 대입할 수 있습니다.

반대의 경우는 Compose Component 같은 경우, onValueChanged 같은 콜백함수를 파라미터로 가지고 있어요. 해당 파라미터는 네이밍 그대로 값이 변경되면 호출되는데, 해당 파라미터를 viewModel의 StateFlow에 value를 초기화하는 함수를 연결 시키는 방향으로 구현 할 수 있어요.

Copy link
Member Author

Choose a reason for hiding this comment

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

fun setManagementCode(code: String) { _manageCode.value = code }

TextField(value = viewModel.state.collectAsState().value, onValueChanged = { value -> viewModel.setManagementCode(value) })
이런식으로 가능합니다 !

@tgyuuAn tgyuuAn added ✏️수정 요청✏️ 코드 리뷰후 코드 수정 요청 and removed 🔥리뷰 기다리는 중🔥 PR을 올리고 코드리뷰를 기다리고 있는 상태입니다. labels Oct 26, 2023
@jeongjaino jeongjaino added 🌟머지 해주세요🌟 코드 리뷰가 완료된 뒤 PR을 올린사람이 Merge를 하면 되는 단계입니다. and removed ✏️수정 요청✏️ 코드 리뷰후 코드 수정 요청 labels Oct 27, 2023
@jeongjaino jeongjaino merged commit c4851d5 into develop Oct 27, 2023
@tgyuuAn tgyuuAn deleted the feature/jaino/#38 branch February 6, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟머지 해주세요🌟 코드 리뷰가 완료된 뒤 PR을 올린사람이 Merge를 하면 되는 단계입니다. 🌱기능🌱 새로운 기능 두두둥장! 🎨유아이🎨 피그마, 엑세멜, 컴포즈 유아이/유엑스 작업 🚀진호🚀 ESTP 정진호 23세
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants