Conversation
angryPodo
left a comment
There was a problem hiding this comment.
고생했습니다! 꽤나 생각할게 많네요. 리뷰 확인하고 노티 부탁드립니다!
| pageCount: Int, | ||
| currentPage: Int, | ||
| modifier: Modifier = Modifier, | ||
| dotWidth: Dp = 8.dp, |
There was a problem hiding this comment.
파라미터가 많기도 하고 dotWidth,lineWidh 와 같은게 어떤 역할인지 내부 코드를 보기전까지 와닿지가 않았어요.
네이밍을 수정하는건 어떨까요? 그리고 디자인시스템으로 강제한만큼 스타일 관련 속성은 Data Class를 활용해서 묶어주는게 좋을 것 같습니다.
data class IndicatorStyle(
val dotWidth: Dp = 8.dp,
val activeLineWidth: Dp = 20.dp,
val height: Dp = 8.dp,
val spacing: Dp = 8.dp,
val activeColor: Color,
val inactiveColor: Color
)There was a problem hiding this comment.
말씀해주신 것처럼 현재 네이밍이 어떤 역할을 하는지 파악하기 어려울 수 있을 것 같아요. 범용 컴포넌트인 만큼 네이밍을 더 명확하게 역할을 전달할 수 있도록 수정해보겠습니다
스타일 관련 속성을 data class로 묶는 것에 대해서는 제가 의도한 방향과는 차이가 있어 고민이 되는데요 현재 저희의 디자인시스템 모듈은 compose스러운 컴포넌트를 지향하고 있다고 생각했고 다른 컴포넌트들 또한 같은 방향으로 구현되어 있는 것으로 보여요 물론 말씀하신 data class를 활용한 방식도 분명히 장점이 있지만 앞서 말씀드린 관점에서 봤을 때 data class로 묶는 것 보다는 파라미터로 받는 형식이 더 compose스럽다고 생각했습니다. 그래서 디자인시스템 내 컴포넌트가 어떤 형태로 구현되어야 한다고 생각하시는지 의견이 궁금합니다
There was a problem hiding this comment.
수정해주신 네이밍 덕에 역할이 훨씬 명확하게 와닿네요! 반영 감사합니다👍🏻
다만 'Compose'스러움에 대해 조금 더 의견을 나누고 싶어요. 효빈님 말씀처럼 개별 파라미터를 노출하는 것이 일반적이긴 하나, M3의 표준 컴포넌트들을 보면 속성이 많아질 때 ButtonColors, ButtonElevation 같은 별도 객체나 Defaults객체를 통해서 스타일을 전달하는게 권장되는 패턴입니다.
물론 다른 컴포넌트들 또한 같은 방향으로 구현되어 있는 것으로 보여요라고 말씀해 주신부분도 맞습니다. 제가 이전까지 가지고 있던 가치관도 파라미터 형식을 선호했으나 최근 관심사 분리, 가독성, 응집성 등등의 관점을 가지게 되어서 이전 구현 방식과 다른 코멘트가 되었어요. 아무래도 리드인 제 스타일이 다른 팀원들에게도 자연스럽게 녹아든 결과가 아닐까 싶습니다😓
그래서 제가 제안드린 방식은 스타일 속성들을 하나의 class로 묶고 현재 설정하신 값을 기본으로 제공하는 형태입니다. 이렇게 하면 핵심 로직과 스타일링 설정이 분리되는 장점과 해당 컴포넌트를 호출할때 가독성과 파악하는 리소스에 장점이 있습니다. 나중에 애니메이션 시간 드으이 새로운 스타일 속성이 추가되어도 메인 함수의 시그니처를 건드리지 않아도 되니 의존도도 낮춰지구요.
기존 컴포넌트들도 이런 방향아로 개선해 나가면 우리 디자인 시스템의 완성도가 더 높아질 것 같은데 효빈이 생각은 어떤가요?
다른 팀원분들도 한번씩 생각해보면 좋은 주제 같습니다.
@nahy-512 @nhyeonii @Daljyeong
There was a problem hiding this comment.
ButtonColors, ButtonElevation같은 객체를 많이 사용하는 것 역시 알고 있습니다. 다만 말씀하신 것처럼 스타일 속성을 하나의 class로 묶는 것과 파라미터로 속성을 받는 것 중 무엇이 더 compose스러운가에 대해 생각해보면 저는 하나의 class로 묶는 것은 compose스러운 것과 거리가 멀다고 느꼈어요 만약 언급하신 M3의 표준 컴포넌트들의 구현 방식을 차용하고 싶다면 스타일 속성을 하나의 class로 묶는 것이 아닌 크기, 색상 등 관련된 스타일들을 나눠서 구현하는 것이 더 m3스럽다고 생각했습니다.
기존 컴포넌트들도 개선해나가면 좋을 것 같다는 의견에 동의합니다. 이를 개선해나가기 위해서는 설계 및 구현에 대해 논의가 더 필요할 것 같아요 디자인 시스템 컴포넌트의 스타일 전달 방식은 지금 이 PR에서 바로 반영하는 것보다 다같이 기준을 정하고 일관되게 적용하는 것이 좋을 것 같은데 논의 후 별도의 이슈로 작업하는 것은 어떨까요?
...src/main/java/com/hilingual/core/designsystem/component/indicator/HilingualPagerIndicator.kt
Outdated
Show resolved
Hide resolved
...src/main/java/com/hilingual/core/designsystem/component/indicator/HilingualPagerIndicator.kt
Show resolved
Hide resolved
data/user/src/main/java/com/hilingual/data/user/repository/UserRepository.kt
Outdated
Show resolved
Hide resolved
core/localstorage/src/main/java/com/hilingual/core/localstorage/OnboardingStateManagerImpl.kt
Show resolved
Hide resolved
presentation/home/src/main/java/com/hilingual/presentation/home/HomeViewModel.kt
Outdated
Show resolved
Hide resolved
presentation/home/src/main/java/com/hilingual/presentation/home/HomeViewModel.kt
Show resolved
Hide resolved
.../src/main/java/com/hilingual/presentation/home/component/onboarding/HomeOnboardingContent.kt
Outdated
Show resolved
Hide resolved
presentation/home/src/main/java/com/hilingual/presentation/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
presentation/home/src/main/java/com/hilingual/presentation/home/HomeViewModel.kt
Outdated
Show resolved
Hide resolved
Daljyeong
left a comment
There was a problem hiding this comment.
다양한 측면에서 많은 신경을 써야 했던 작업이었네요..! 넘 수고하셨습니다 🥹
코드가 조금 더 수정될 것 같아 추후 팔로업하도록 하겠습니다 :)
| } | ||
|
|
||
| HilingualPagerIndicator( | ||
| pageCount = 4, |
There was a problem hiding this comment.
위에서는 onboardingPages.size가 사용된 것 같은데, 여기에서는 4라고 따로 명시해서 구현하신 이유가 있는지 궁금합니다!
There was a problem hiding this comment.
그것은 저의 실수입니다.. 하핫 onboardingPages.size로 수정되었습니다!
nahy-512
left a comment
There was a problem hiding this comment.
수고하셨습니다! 바텀시트 배경 클릭 한번 확인해주시면 좋을 것 같습니다
...src/main/java/com/hilingual/core/designsystem/component/indicator/HilingualPagerIndicator.kt
Show resolved
Hide resolved
| activeIndicatorWidth: Dp = 20.dp, | ||
| inactiveIndicatorWidth: Dp = 8.dp, |
There was a problem hiding this comment.
dot, line 대신 명칭 active/inactive로 관리하니까 훨씬 좋은 것 같아요!
사소한 부분이긴 한데, 아래 indicatorHeight, indicatorSpacing과 위치 한번 바꿔주면 조금 더 보기 편할 것 같습니다:)
인디케이터 색상도 active/inactive로 구분해주고 있는 것 같아서요
| @Composable | ||
| private fun PagerContent( | ||
| text: String, | ||
| image: Int |
There was a problem hiding this comment.
@DrawableRes image: Int
로 써주시면 조금 더 명확할 것 같습니다:)
| ) { | ||
| HilingualBasicBottomSheet( | ||
| isVisible = isVisible, | ||
| onDismiss = {}, |
There was a problem hiding this comment.
해당 설정만으로는 dim 영역 클릭했을 때 바텀시트가 내려가는 것 같은데, 'dim 영역 클릭해도 내려가지 않음' 이라는 정책상 위배되는 것 같아 확인 부탁드리겠습니다!
sheetState에서 confirmValueChange를 따로 설정해줘야할 것 같아요
There was a problem hiding this comment.
// Source - https://stackoverflow.com/a/79707792
// Posted by ΓDΛ
// Retrieved 2026-02-11, License - CC BY-SA 4.0
ModalBottomSheet(
onDismissRequest = onDismiss,
properties = ModalBottomSheetProperties(
shouldDismissOnClickOutside = false
)
) {
properties를 통해서 배경 클릭 시 dismiss 여부를 설정할 수 있네요
HilingualBasicBottomSheet 프로퍼티를 수정할 필요가 있을 것 같아요.
There was a problem hiding this comment.
엇 그러네요! 수정하겠습니다ㅎㅎ
angryPodo
left a comment
There was a problem hiding this comment.
반영하신것 확인했습니다ㅎㅎ 답변 확인 부탁드릴게요💪🏻
nhyeonii
left a comment
There was a problem hiding this comment.
제가 피알 확인이 늦었네요 죄송합니다😭 도메인 로직과 UX 상태를 구분해서 별도의 Manager로 분리한 판단이 인상깊었어요 ! 고생하셨습니다 !!
| sheetState = rememberModalBottomSheetState( | ||
| skipPartiallyExpanded = true, | ||
| ), | ||
| isDimEnabled = true, |
There was a problem hiding this comment.
isDimEnabled는 기획 명세에 적혀있는 부분이라 작성해두었습니다! 단순히 기본값으로 사용하는게 아니라 의도가 담긴 속성이라서요 근데 sheetState는 저도 왜 들어갔는지 모르겠네요...? 이건 빼두도록 하겠습니다!
| modifier = Modifier | ||
| .fillMaxWidth() |
There was a problem hiding this comment.
| modifier = Modifier | |
| .fillMaxWidth() | |
| modifier = Modifier.fillMaxWidth() |
요기 속성 하나니까 띄어쓰기 안하는거 어떠신가용 ?!
| sheetState: SheetState = rememberModalBottomSheetState( | ||
| skipPartiallyExpanded = true | ||
| ), | ||
| properties: ModalBottomSheetProperties = ModalBottomSheetProperties(), |

PR chekList
Related issue 🛠
Work Description ✏️
Screenshot 📸
Screen_Recording_20260209_175719.mp4
Uncompleted Tasks 😅
N/A
To Reviewers 📢
로컬 스토리지에 저장하고 회원가입 완료된 후에만 isHomeOnboardingCompleted를 false로 바꾸도록 구현했어요! 기존 유저가 다른 기기에서 로그인하는 경우 온보딩이 노출되지 않도록 하기 위해 기본값은 true로 설정해두었습니다 또한 온보딩 관련 플래그들은 유저, 다이어리 등과 같이 도메인 정보가 아닌 앱 ux에 대한 상태라고 생각해서 별도의 OnboardingManager를 구현했습니다~!
그리고!! 인디케이터는 기본 컴포넌트 중 하나라 디자인시스템으로 추가했는데 이견 있으시면 슬쩍 말씀해주시면 바로 ui로 옮기겠습니다ㅎㅎ