Conversation
vvan2
left a comment
vvan2
left a comment
There was a problem hiding this comment.
우지 열심히 했네, 오랜만에 안드하는 모습 보니까 아주 씩씩하네요
뷰 잘 만들어줬고 수정사항 몇 개만 확인해주잉 도키하하
| shape = RoundedCornerShape(8.dp) | ||
| ) { | ||
| Text( | ||
| text = text, |
There was a problem hiding this comment.
typo Design system에 있는 subbuttonactive 로 변경 부탁드릴게용~
| fontWeight = FontWeight.Medium, | ||
| color = PawKeyTheme.colors.primary, | ||
| style = PawKeyTheme.typography.subButtonActive, | ||
| modifier = Modifier.padding(8.dp) |
There was a problem hiding this comment.
텍스트 기준으로 내부 padding 줄려고 Text 컴포져블에서 준 것 같네요, chip은 surface로 안감싸고 Text 만 사용해서 만들 수 있긴한데, 지금도 잘만들어서 그냥 그렇게 만들수도 있다 정도만.. 하하
| import com.paw.key.core.designsystem.theme.PawKeyTheme | ||
|
|
||
| // TraitAnalysis 데이터 클래스 정의 | ||
| data class TraitAnalysis( |
There was a problem hiding this comment.
UI 계층에서 사용되는 data class 같은 경우 component 파일보다는 model 패키지를 만들어서 따로 관리해주는게 좋아보여요! 여러 model 사용시 해당 파일에서만 관리해도 되고 컴포넌트만을 관리하는 해당 파일에서 UI 만 관리하는 역할만 한다면 더 좋을 것 같습니당
There was a problem hiding this comment.
API 연동 전이라 임시로 component에 두었는데, 연동하면서 model 패키지를 만들어서 이동하겠습니당~~
| type: String, | ||
| name: String, | ||
| imageUrl: String?, | ||
| keywords: List<String>, |
There was a problem hiding this comment.
keywords 랑 analysis 파라미터를 사용할 때 List 로 받는다면 unstable한 상태로 불필요한 리컴포지션이 일어날 수 있어서 ImmutlabeList 로 변경하면 좋을 것 같습니다!
| modifier = modifier.fillMaxWidth(), | ||
| color = PawKeyTheme.colors.background, | ||
| shape = RoundedCornerShape(8.dp) | ||
| ) { |
There was a problem hiding this comment.
여기도 여러 컴포져블 함수를 많이 사용해서 depth가 깊어지는 것 같아서,
해당 부분만 하나의 영역으로 묶고 Column 하나만 사용해서 구현할 수 있을 것 같아용
| imageUrl: String?, | ||
| topText: String, | ||
| bottomText: String, | ||
| isSelected: Boolean = false, |
There was a problem hiding this comment.
파라미터 순서 지켜주시면 더 좋을 것 같에용
필수인자 - modifier - 선택인자
|
|
||
| Column( | ||
| modifier = modifier | ||
| .aspectRatio(159.5f / 219.31f) // 비율로 크기 조정 |
There was a problem hiding this comment.
asepctRatio 쓸 때 피그마에 있는 크기를 사용해도 되지만 말 그대로 비율로 조정하는 것이기 때문에 지금 꺼는 (8f/11f)로 사용해도 될듯요!
There was a problem hiding this comment.
여기도 위쪽 내용이랑 중복된게 많아서 중복내용 몇 가지만 고쳐주세욥
| ) { | ||
| var selectedOptionId by remember { mutableStateOf<Int?>(null) } | ||
|
|
||
| Box(modifier = modifier.fillMaxSize()) { |
| var selectedOptionId by remember { mutableStateOf<Int?>(null) } | ||
|
|
||
| Box(modifier = modifier.fillMaxSize()) { | ||
| Column(modifier = Modifier.fillMaxSize()) { |
There was a problem hiding this comment.
피그마에 있는 해당 뷰의 padding 요소가 일관된 느낌이 아니라서 spacer를 사용해준 것 같은데, Alignment.spacedBy로 내부 요소간의최소 padding 영역을 잡아주고 필요한 곳에만 spacer 를 사용해주면 좋을 것 같습니당
sonms
left a comment
sonms
left a comment
There was a problem hiding this comment.
오랜만에 우지님의 pr을 보니 너무 좋네요 ㅎㅎ 다시 힘내보자
주완님이 씩씩하게 적어놓으셔서 저는 어푸만 누르겠습니당 대 주 완, 대 지 우
3 participants
ISSUE
❗ WORK DESCRIPTIONAdd commentMore actions
📸 SCREENSHOT
📢 TO REVIEWERS