-
Notifications
You must be signed in to change notification settings - Fork 1
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/#94 #111
Feature/jaino/#94 #111
Conversation
…re/jaino/#94 # Conflicts: # feature/management/src/main/java/com/wap/wapp/feature/management/ManagementGuestScreen.kt # feature/management/src/main/java/com/wap/wapp/feature/management/ManagementViewModel.kt
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.
진호상 고생했어요!
취업 전 마지막 주에 누가 과연 스프린트를 돌리고 있을까요...
너무 고생하셨습니다. 👍👍👍😁😁😁
|
||
internal fun getSeoulDateTimeNow() = LocalDateTime.now(ZoneId.of("Asia/Seoul")) |
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.
👍👍👍
fun getUserRole() { | ||
viewModelScope.launch { | ||
getUserRoleUseCase() |
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.
여기 바로 = 할 수 있을 것 같아요!
} | ||
} | ||
|
||
val isFirstQuestion = questionNumber > 0 |
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.
isFirestQuestin
이면,
첫 번째 항목임을 물어보는 것 같은데
questionNumber > 0
이 0보다 클 때만 True를 뱉으므로,
사실 변수명과 반대로 로직이 돌아가는 것 아닐까요?!
val isFirstQuestion = questionNumber == 0
이지 않을까요?!
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.
오호 완전히 제대로 반대로 잡았네요 ㄷㄷㄷ 수정해보겠습니다
questionType: QuestionType, | ||
subjectiveAnswer: String, | ||
): Boolean { | ||
if (questionType == QuestionType.SUBJECTIVE) return subjectiveAnswer.length >= 10 |
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.
여기 스코프를 넣어주는 것 보다 안넣어주는 것이 더 낫나요 ?!
사실 클린 코드책에서는 이렇게 단일 식이더라도 스코프를 넣어라고 말하긴 하는데,
이건 명확한 답이 없기 때문에 진호님의 의견이 궁금해요!
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.
아마 블락을 사용하는게, 가독성 면에서도 확실히 다른 로직과 구분이 돼서 좋을 것 같아요 !
뒤에도 따로 복잡한 로직이 안들어 있어서, 단순히 코드 길이를 줄이기 위해서 다음과 같이 구현했어요 !
코드 길이와 가독성 측면에서의 항상 선택을 해야하는 것 같아요
@@ -53,7 +42,7 @@ internal fun SurveyAnswerScreen( | |||
viewModel.surveyAnswerEvent.collectLatest { | |||
when (it) { | |||
is SurveyAnswerViewModel.SurveyAnswerUiEvent.SubmitSuccess -> { |
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.
여기는 스코프 빼도 될 것 같아요!
contentWindowInsets = WindowInsets(0.dp), | ||
topBar = { | ||
WappSubTopBar( | ||
titleRes = R.string.survey_answer, | ||
showLeftButton = true, | ||
modifier = Modifier.padding(top = 16.dp), // 하단은 Content Padding에 의존 | ||
onClickLeftButton = { | ||
when (surveyAnswerState) { | ||
SurveyAnswerState.SURVEY_OVERVIEW -> navigateToSurvey() | ||
SurveyAnswerState.SURVEY_ANSWER -> viewModel.setSurveyAnswerState( | ||
SurveyAnswerState.SURVEY_OVERVIEW, | ||
) |
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.
👍👍👍👍😁😁
when (surveyAnswerState) { | ||
SurveyAnswerState.SURVEY_OVERVIEW -> { | ||
SurveyOverview( | ||
surveyForm = surveyForm, | ||
modifier = modifier.padding(top = 16.dp), | ||
eventName = eventName, | ||
onStartSurveyButtonClicked = onStartSurveyButtonClicked, | ||
) | ||
} | ||
|
||
val isLastQuestion = questionNumber == lastQuestionNumber // 마지막 응답일 경우, 완료로 변경 | ||
SurveyAnswerButton( | ||
isLastQuestion = isLastQuestion, | ||
onButtonClicked = onNextButtonClicked, | ||
isEnabled = isButtonEnabled(surveyQuestion.questionType, subjectiveAnswer), | ||
) | ||
} | ||
} | ||
|
||
@OptIn(ExperimentalMaterial3Api::class) | ||
@Composable | ||
private fun SurveyAnswerTopBar( | ||
onBackButtonClicked: () -> Unit, | ||
) { | ||
CenterAlignedTopAppBar( | ||
title = { | ||
Text( | ||
text = stringResource(R.string.survey_answer), | ||
textAlign = TextAlign.Center, | ||
modifier = Modifier.fillMaxWidth(), | ||
style = WappTheme.typography.contentBold, | ||
color = WappTheme.colors.white, | ||
SurveyAnswerState.SURVEY_ANSWER -> { | ||
SurveyAnswerForm( | ||
surveyForm = surveyForm, | ||
modifier = modifier, | ||
questionNumber = questionNumber, | ||
subjectiveAnswer = subjectiveAnswer, | ||
objectiveAnswer = objectiveAnswer, | ||
onSubjectiveAnswerChanged = onSubjectiveAnswerChanged, | ||
onObjectiveAnswerSelected = onObjectiveAnswerSelected, | ||
onNextQuestionButtonClicked = onNextQuestionButtonClicked, | ||
onPreviousQuestionButtonClicked = onPreviousQuestionButtonClicked, |
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.
위에 when절은 개행이 없는데,
SURVEY_ANSWER은 개행이 하나 들어가있어요.
when절의 시작 구문은 개행이 필요없지 않을까요?!
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.
그럼요 !!!
private fun getEvent() { | ||
viewModelScope.launch { | ||
getEventUseCase(eventId = _surveyForm.value.eventId) | ||
.onSuccess { event -> | ||
_eventName.value = event.title |
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.
여기도 바로 =
넣을 수 있을 것 같아욥
fun modifySurveyAnswer() { | ||
val questionNumber = _questionNumber.value | ||
val surveyQuestion = _surveyForm.value.surveyQuestionList[questionNumber] | ||
val surveyAnswerList = _surveyAnswerList.value | ||
|
||
when (surveyQuestion.questionType) { // 새로운 질문에 답변을 작성하는 경우 | ||
QuestionType.SUBJECTIVE -> { | ||
surveyAnswerList[questionNumber] = SurveyAnswer( | ||
questionType = surveyQuestion.questionType, | ||
questionTitle = surveyQuestion.questionTitle, | ||
questionAnswer = _subjectiveAnswer.value, | ||
) | ||
clearSubjectiveAnswer() // 질문 상태 초기화 | ||
} | ||
|
||
QuestionType.OBJECTIVE -> { | ||
surveyAnswerList[questionNumber] = SurveyAnswer( | ||
questionType = surveyQuestion.questionType, | ||
questionTitle = surveyQuestion.questionTitle, | ||
questionAnswer = _objectiveAnswer.value.toString(), | ||
) | ||
clearObjectiveAnswer() | ||
} | ||
} | ||
} |
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.
modfiy
는 수정하다인가요 ?!
기존 패키지명으로 사용하고 있던 edit이나 HTTP 메소드로 사용되는 Update랑은 다른 개념일까요?
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.
마지막 화면에서 작성하고, 이전 버튼을 누르면 상태가 저장되지 않고 날라가는 버그 발견 |
이번 PR에서 해결하시나요?! |
그럼요 ~ |
태규상 변경내용 수정하고, 버그도 해결했습니다잇 ! |
…re/jaino/#94 # Conflicts: # core/network/src/main/java/com/wap/wapp/core/network/source/attendancestatus/AttendanceStatusDataSourceImpl.kt # core/network/src/main/java/com/wap/wapp/core/network/source/event/EventDataSourceImpl.kt # core/network/src/main/java/com/wap/wapp/core/network/utils/LocalDateTime.kt
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.
LGTM 입니다 진호상.
몇개 잔잔바리들만 코멘트 남겨놨씁니다. 👍👍
이제 해당 기능에 자신감 100퍼신가요?
viewModel.getUserRole() // 유저 권한 검색 | ||
|
||
viewModel.userRole.collectLatest { managerState -> | ||
when (managerState) { | ||
UserRole.GUEST -> { showGuestScreen = true } | ||
UserRole.MEMBER -> { showValidationScreen = true } | ||
UserRole.MANAGER -> viewModel.getEventSurveyList() | ||
viewModel.userRole.collectLatest { userRoleUiState -> |
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.
viewModel.apply{
}
를 이용해서 반복되는 viewModel을 제거하는 것은 별로일까요?
근데 사실 2개 밖에 없어서 더 안 좋아보일 것 같기도 하고..🙃🙃
) { | ||
Column( | ||
modifier = Modifier.fillMaxWidth(), |
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.
👍👍👍
if (isLastQuestion) { | ||
WappButton( | ||
textRes = R.string.submit, | ||
onClick = { onButtonClicked() }, | ||
isEnabled = isEnabled, | ||
modifier = modifier, | ||
) | ||
} else { | ||
WappButton( | ||
textRes = R.string.next, | ||
onClick = { onButtonClicked() }, | ||
isEnabled = isEnabled, | ||
modifier = modifier, | ||
) | ||
} |
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.
if (isLastQuestion)
부분이
지금 textRes
빼고는 나머지가 다 같은 것 같은데 textRes 안에서 분기문을 줘도 될 것 같아요!
다음 아슈에서 언급하신부분 수정하는 걸로 하겠습니다 ! |
완한드레드파센타입니다 |
1. 📄 관련된 이슈 및 소개
#94 설문 정보 소개 페이지 구현
2. 🔥 변경된 점
설문 응답 페이지 -> 설문 정보 페이지 -> 설문 홈 의 전환이 되도록 구현
이전 항목 답변 수정 기능 구현
이전 보내드린 영상 참고 부탁드립니다잇!
설문 응답 페이지 컬럼 속성 변경
3. ✅ 꼭 확인해줬으면 하는 점
SurveyAnswerScreen
중요 로직 설명
SurveyAnswerViewModel
Screen(View)에서 해당 setSurveyAnswer를 각각 호출하지 않고, ViewModel에서 호출한 이유
4. 📸 스크린샷(선택)
5. 💡알게된 혹은 궁금한 사항들