Conversation
…into feat/#96-today-status
seungjae708
left a comment
There was a problem hiding this comment.
교수님 진도가 역시 빠르시네요 고생하셨습니다!
점선은 맛있게 먹도록 할게요
There was a problem hiding this comment.
저두 점선이 필요하긴 한디 가로 버전이라서
KieroDashedHorizontalDivider로 하나 만들어볼게여
There was a problem hiding this comment.
참고하셔서 해당 컴포넌트에 방향 전환이 가능하도록 리팩토링을 하여 재사용하거나 다른 기능이라고 생각하셔 새로운 컴포넌트를 만드실 지 궁금하네요 ㅎㅎ
| Text( | ||
| text = "2개", | ||
| style = KieroTheme.typography.semiBold.title4, | ||
| color = KieroTheme.colors.white | ||
| ) |
There was a problem hiding this comment.
p2) 완료/미완료 개수가 하드코딩되어 있는 것 같아요
There was a problem hiding this comment.
하드코딩되어있는 경우 모두 서버 api 연결하면서 변경될 예정입니다!
| data class KidInfo( | ||
| val kidId: String = "", | ||
| val kidName: String = "", | ||
| val currentDate: String = "" | ||
| ) | ||
|
|
||
| fun ChildrenModel.toUiModel() = KidInfo( | ||
| kidId = childId.toString(), | ||
| kidName = childFirstName | ||
| ) |
There was a problem hiding this comment.
p3) currentDate는 매핑이 안되어있는데 얘는 서버에서 안주고 있어서 빠진걸까요?
There was a problem hiding this comment.
아직 서버 구조를 파악하지 못해서 우선은 빼뒀는데 자체 구현인지 아니면 서버에서 내려주는 것인지 확인하고 수정하도록 하겠습니다!
| sealed interface ParentJourneySideEffect { | ||
| object NavigateUp : ParentJourneySideEffect | ||
| } |
There was a problem hiding this comment.
p2) data object 쓰는게 좋을 것 같아용
| .onSuccess { response -> | ||
| val kidInfo = response.firstOrNull()?.toUiModel() | ||
| if (kidInfo != null) { | ||
| _state.update { currentState -> | ||
| currentState.copy( | ||
| kidInfo = kidInfo | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
p3) onFailure도 만들어 두는게 어떨까요
There was a problem hiding this comment.
넵! 저도 만드는 편이 더 ux상 좋아보이네요 추가하겠습니다!
| @Composable | ||
| private fun ParentJourneyScreen( | ||
| paddingValues: PaddingValues, | ||
| navigateUp: () -> Unit, | ||
| state: ParentJourneyState, | ||
| ) { | ||
| Column ( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .background(KieroTheme.colors.black) | ||
| .padding(paddingValues) | ||
| ) { |
There was a problem hiding this comment.
p2) ParentJourneyScreen 컴포저블 함수의 파라미터로
modifier: Modifier = Modifier가 없어서 열어두는게 좋을 것 같아요
There was a problem hiding this comment.
Screen은 외부에서 재사용되거나 layout 제어가 필요한 구조가 아니라서 modifier를 넘기지 않았습니다!
| @Composable | ||
| fun GlowingDot( | ||
| modifier: Modifier = Modifier, | ||
| coreColor: Color = KieroTheme.colors.schedule1, | ||
| glowColor: Color = KieroTheme.colors.schedule1, |
vvan2
left a comment
There was a problem hiding this comment.
안그래도 민성씨꺼 달거 몇개 없는데, 짜이승지가 달아줬넹 고생했으요 어푸푸!
| VerticalDivider( | ||
| modifier = Modifier.fillMaxHeight(), | ||
| color = KieroTheme.colors.gray700, | ||
| thickness = 2.dp |
| Icon( | ||
| imageVector = ImageVector.vectorResource(R.drawable.ic_arrow_right), | ||
| contentDescription = null, | ||
| tint = KieroTheme.colors.white |
There was a problem hiding this comment.
p5) 단순 궁금중이긴 한데, 저도 Icon 설정 할때
Unspecified 쓰거나 DS 에 있는 색상으로 사용할지 가끔 고민을 합니다.
디자인 의도대로라면 해당 color 에 맞춰서 설정해주는게 맞긴한데, 지금 섞어서 사용하는 부분이 많은 거 같아서, 어떤 방향으로 통일할까요?
There was a problem hiding this comment.
저는 요건 자유라고 생각해요!
의도적으로 컨벤션을 맞추기 보다는 하나의 drawable을 tint로 변경하여 여러곳에 적용 가능할 수 있게 하는 것이 재사용성도 높고 재활용도 가능하니까 다른 아이콘을 추가하지 않아 용량에 이점이 생기니까요!
굳이 아이콘의 색상을 변경할 필요가 없다면 Unspecified을 사용해도 좋구요!
| Row ( | ||
| modifier = modifier | ||
| .fillMaxWidth() | ||
| .height(IntrinsicSize.Min), |
There was a problem hiding this comment.
Min과 Max가 있어요!
둘 다 row 안에 포함된 자식 요소들이 보일 필요 높이값을 계산해서 보여주기 위함인데
Min은 자식 요소가 최소한의 모양을 유지할 수 있는 높이
Max는 자식 요소가 가질 수 있는 최대한의 고유한 높이
입니다잉
dmp100
left a comment
There was a problem hiding this comment.
궁금한거 몇개는 달고 가봐용 궁금한거 몇개는 달고 가봐용 흠잡을데가 거의 없네요 역시 👍👍흠잡을데가 거의 없네요 역시 👍👍
| ) { | ||
| val isUpcoming = item.todayStatus == TodayStatus.UPCOMING | ||
|
|
||
| val statusIcon = when (item.todayStatus) { |
There was a problem hiding this comment.
p5 ) statusIcon, statusTextColor, cardBackground 등 상태 파생 로직이 각각 when으로 분리되어 있는데, 하나로 묶지 않고 명시적으로 드러낸 이유가 있나요? 컴포넌트 내 상태 흐름을 각 프로퍼티별로 명확히 보여주기 위한 의도인지 궁금해서요!
There was a problem hiding this comment.
맞습니다!! 핵심을 찌르셨네요 ㅎㅎ
모든 UI 속성이 todayStatus 하나에 의해서 결정된다면 data class 가 좋지만
각 프로퍼티 마다 의존하는 조건이 달라서..
그렇기에 특정 요소의 상태 로직만 찾아 수정하기 편하도록 가독성과 유지보수성을 고려해 명시적으로 분리했습니다!!


ISSUE
❗ WORK DESCRIPTION
📢 TO REVIEWERS
점선용 KieroDashedVerticalDivider 을 공통으로 만들어놨습니다 나중에 사용하실 거면 이거 사용하시면 됩니다!
나머지는 서버 구현하면서 세부적으로 수정하거나 반영하겠습니다
GlowingDot을 현재 뷰에서만 사용한다고 생각하여 private를 붙여 StatusItem에 구현하였으나 추후 만약 다른 뷰에서도 사용된다면 분리하도록 하겠습니다
아직은 얘기가 없어서리..
📸 SCREENSHOT