-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/#225 : 홈화면 UI 변경건 작업 #226
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
base: develop
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderrabbit review |
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: resource 중에서 svg로 추출되는게 없었나용?
| rightIcon: @Composable () -> Unit = { | ||
| Row( | ||
| modifier = Modifier | ||
| .fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.End | ||
| ) { | ||
| Icon( | ||
| modifier = Modifier.yappClickable(onClick = onClickRightIcon), | ||
| painter = painterResource(R.drawable.icon_setting), | ||
| contentDescription = "setting", | ||
| ) | ||
| } | ||
| } |
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 sessionListWithNotice = homeSessions.upcomingSessionId | ||
| ?.takeIf { it.isNotBlank() } | ||
| ?.let { sessionId -> | ||
| runCatchingIgnoreCancelled { | ||
| scheduleRepository.getSessionDetail(sessionId) | ||
| }.onFailure { e -> | ||
| when (e) { | ||
| is InvalidTokenException -> postSideEffect(HomeSideEffect.NavigateToLogin) | ||
| else -> { | ||
| postSideEffect(HomeSideEffect.HandleException(e)) | ||
| e.record() | ||
| } | ||
| } | ||
| }.getOrNull()?.let { detail -> | ||
| homeSessions.copy(upcomingNotice = detail.notices) | ||
| } | ||
| } ?: homeSessions |
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: 개인적으로는 중괄호랑 들여쓰기가 많아서 가독성이 조금 떨어지는 느낌이네용
시간 나면 한번 개선해주세용
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.
요건 내비게이션 처리하면서 클래스로 묶거나 함수처리로 변경할게여 ~
|
Yapp 기본 규칙, N기 커리큘럼 네비게이션은 찬기님께 여쭤보시고 후속 작업 부탁드릴게요! |
몇가지는 음영이 있었던걸로 기억해 svg 로 추출하면 의도와 다르게 노출될거라 생각해서 최대한 이미지로 뽑아냈었어여 |
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.
수고하셨습니다!
R로 표시한 부분은 피그마 디자인과 다르거나 로직상 문제로 보이는 부분이고
그 이외는 제 개인적인 의견이여서 반영해주셔도 안 반영해주셔도 괜찮습니다!
실수로 Approve로 체크했으나 변경 요청 상태로 봐주시면 감사하겠습니다
| OtherSection( | ||
| modifier = Modifier.padding(horizontal = 16.dp), | ||
| clickAttendance = { onIntent(HomeIntent.ClickShowAllAttendanceHistory) }, | ||
| clickTotalSession = { onIntent(HomeIntent.ClickShowAllSession) }, | ||
| ) | ||
| Spacer(Modifier.height(16.dp)) | ||
| FAQSection( | ||
| modifier = Modifier.padding(horizontal = 16.dp), | ||
| clickCurriculum = {}, | ||
| clickBasicRule = {} | ||
| ) |
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.
내부 컴포넌트랑 마진이 헷갈렸네요
홈 헤더 컴포저블도 외부에서 패딩값을 주는형태로 변경했습니다
| verticalAlignment = Alignment.CenterVertically, | ||
| ) { | ||
| Image(painter = painterResource(coreDesignR.drawable.yappo_wink), contentDescription = null) | ||
| Text(text = "YAPP 기본 규칙") |
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.
R:
| Text(text = "YAPP 기본 규칙") | |
| Text( | |
| text = "YAPP 기본 규칙", | |
| style = YappTheme.typography.body1NormalRegular, | |
| color = YappTheme.colorScheme.labelNormal | |
| ) |
R: YAPP 기본 규칙이랑 N기 커리큘럼 글꼴 스타일 적용해주세요
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.
처리완료 했어요
| YappSolidPrimaryButtonLarge( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| text = parsedDate?.let { |
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.
| YappSolidPrimaryButtonLarge( | |
| modifier = Modifier.fillMaxWidth(), | |
| text = parsedDate?.let { | |
| if (session != null) { | |
| YappSolidPrimaryButtonLarge( | |
| modifier = Modifier.fillMaxWidth(), | |
| text = parsedDate?.let { |
R: 다음 세션 정보가 없다면 M월 D일 세션 예정, 다가오는 세션이 없어요 를 표시 안하기 때문에 session(=upcomingSession) 이 null이 아닐때만 표시하게 수정해야 할 것 같아요
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.
이거는 UpcomingSessionCard 가 역할이 점점 커지는거 같아 아예 로직 분리 처리를 호이스팅 시켜서
캡쳐해주신 세션 카드 컴포넌트를 만들어서 구분 처리했어요
| val duration = remember( | ||
| configuration, | ||
| session | ||
| ) { | ||
| formatTimeRange( | ||
| context = context, | ||
| startTime = session.startTime, | ||
| endTime = session.endTime, | ||
| ) | ||
| } |
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: remember로 감싼 거 좋네요 👍
| } | ||
| } | ||
| } | ||
|
|
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: Preview 추가해주시면 작업할 때 편할 것 같아요
@Preview
@Composable
private fun PreviewHomeAttendanceContent() {
YappTheme {
HomeAttendanceContent(
upcomingSession = null,
notices = emptyList(),
onClickAttend = { },
onClickNotice = { _ -> }
)
}
}| stringResource(R.string.session_time_end, displayTime) | ||
| else | ||
| stringResource(R.string.session_time_open, displayTime), | ||
| text = formatStartTime(context, session.startTime), |
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: 사소하긴 한데 컴포지션때마다 formatStartTime 재호출을 막기 위해서 remember(session.startTime) 으로 감싸도 좋을 거 같아요!
| onClickNotice = onClickNotice, | ||
| ) | ||
| } | ||
| } |
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: Preview 추가해주시면 작업할 때 편할 거 같아요
@Preview
@Composable
private fun PreviewCurrentSessionSection() {
YappTheme {
CurrentSessionSection(
upcomingSession = null,
todaySession = null,
notices = emptyList(),
onClickAttend = { },
onClickDetail = { _ -> },
onClickNotice = { _ -> }
)
}
}
💡 Issue
🌱 Key changes
홈 UI 가 전반적으로 변경되었습니다.
HomeViewModel 을 중점으로 리뷰해주시면 감사하겠습니다.
✅ To Reviewers
Yapp 기본 규칙, N기 커리큘럼은 어디로 내비게이션 처리를 해야할지 몰라서 일단은 비어둔채로 진행했습니다.
📸 스크린샷
Screen_recording_20251021_201651.mp4