Conversation
Test Results19 tests 19 ✅ 5s ⏱️ Results for commit 36a2ae7. ♻️ This comment has been updated with latest results. |
|
|
||
| @Composable | ||
| fun CircularNetworkImage( | ||
| size: Dp, |
There was a problem hiding this comment.
modifier가 외부로 나와있는 상태라 size를 추가로 전달한다면 Modifier.size()처리하는것과 겹치게 됩니다.
modifier가 이미 있기 때문에 불필요한 부분으로 보여지네요.
적한한 방법을 한번 더 고민해보면 좋을것 같은데요.
modifier를 제공하지 않는것을 고려해도 좋을것 같네요.
| KnightsTheme { | ||
| SpeakerName("홍길동") | ||
| } | ||
| } |
There was a problem hiding this comment.
이렇게까지 분리할 필요가 있나요? UI가 변경되었을때 타이틀 부분만 변경되어지는건 아닐것 같은데
과도한 분리로 보이네요. 분리했을때의 명확한 장점이 무엇일까요?
제가 생각한 컴포넌트 분리라면 BookmarkCard 카드 하나면 이미 끝난것 같은데. 그럼 카드를 그대로 가져다 사용할 수 있는데, 각각을 전부 분리했다면 어떤 장점이 있는지 전혀 모르겠네요.
함수 하나에 Text 하나일 뿐인데
|
@tmdgh1592 작업해주신 내용은 정말 감사합니다. 그리고 컴포넌트라함은 재사용이 가능해야합니다. 현재의 함수 나눈 부분은 재사용이 1도 불가능합니다. 그냥 하나의 리스트에 포함되어질 컴포넌트를 Text 단위로 분리했을 뿐이지 이걸 다른곳에서 재사용이 전혀 불가능하고, 수정을 한다고 하더라도 리스트 UI가 새로운 형태로 만들어지면 수정의 범위가 넌무 넓어지는 형태입니다. 우리가 함수를 나누었을때의 장점이 있는데, 함수를 많이 나눈다고 명확하진 않습니다. 목적이 분명해야합니다. 죄송하지만 목적에 맞는 분리를 생각하고, 분리를 해보자는 티켓을 작성한것인데요. 아쉽지만 현재 구조로는 머지가 불가능합니다. |
|
@taehwandev 시간내서 리뷰해주셔서 감사합니다. 리뷰를 보고 나니 제가 생각한 장점보다 코드를 수정할 때 느낄 불편함이 조금 더 커보이네요,, 기존 코드를 바탕으로 재사용 가능성 or 기능별 분리에 대해 다시 생각해보고 리뷰요청을 드려도 될까요? |
|
@tmdgh1592 보통 스크린 단위 하나, 컴포넌트 하나에서의 다양한 변화를 직접적으로 보고, 수정하는게 좋을것 같단 생각이 드네요. 길어져서 Text 하나를 분리했다곤 하지만 지금의 코드상 Title 하나, SubMessage 하나 이런식이라서 나중에 수정을 한다면 그걸 다 찾아서 수정을 해줘야 하는 단점도 있습니다. 일단 제가 원래 의도한 단위별로 분리는 이와 같았는데 충분히 설명하지 못했던것 같네요.
란 정도로만 생각하고 분리해보자란 글을 올렸습니다. Text 도 하나하나 분리하면 충분히 의미가 없는건 아니겟지만, 조합으로 본다면 이미 Text는 분리되어있고, 이를 가져다 재사용하는 구조인데, 이걸 다시 구조화 하려고 분리했다면... 적절할까? Style 기준으로 분리하는것이 적절할 것인가? 함수를 많이 나누는것도 적절한 의미가 있는것이 좋을것 같습니다. 다시 한번 고민 부탁드립니다. |
|
@taehwandev 말씀해주신 내용들 모두 공감합니다. |
|
@taehwandev 안녕하세요! 말씀해주신 내용들 반영해서 Text, Image와 같은 부분들은 원복시켜두었습니다. +) 기존 main브랜치 코드에 ex) @Composable
private fun SessionDetailTitle(
title: String,
modifier: Modifier = Modifier,
) {
Text(
modifier = modifier.padding(end = 58.dp),
text = title,
style = KnightsTheme.typography.headlineMediumB,
color = MaterialTheme.colorScheme.onSecondaryContainer,
)
} |
taehwandev
left a comment
taehwandev
left a comment
There was a problem hiding this comment.
@tmdgh1592
원래의 목적인 UI 파일을 분리하는 부분과 shimmer 적용, Preview 적용까지 하나의 PR에 모두 있어서 살펴보는 시간이 너무 많이 걸립니다.
목적에 맞고, 단위단위로 PR이 쪼개져야 할것 같네요.
단위별로 분리인데 단위 분리보단 다른 내용이 더 많아 보입니다. PR 분리를 요청드립니다. 이 상태로는 계속 끝나지 않고 남아있을것 같네요.
적절한 분리를 부탁드립니다.
그나마 가장 괜찮은 분리라면
- 이후 공통 모듈을 추가하신 부분을 먼저 PR.
- 공통 모듈을 추가하신 이유를 함께 설명한다. shimmer 역시 너무 불필요한 분리로 보여지네요. 컴포넌트별로 다 만들 이유가 없는 Modifier를 활용하도록 만들었는데, 이걸 다시 한번 컴포넌트별로 분리한 이유를 모르겠군요. 아마 shimmer안에도 옵션을 제공하는걸로 아는데요. 그걸 활용하는게 더 적절합니다.
- 모듈별로 컴포넌트 분리를 별도 PR로 올린다.(이때 Preview까지 포함하거나, 별도로 분리하거나)
|
@taehwandev 추천해주신 PR 분리 방법으로 PR : +) 이전 커밋으로 돌리려고 포스 푸시를 하다가 PR이 닫혀서 위 순서대로 새로 PR을 요청하겠습니다! |
2 participants
Issue
Overview (Required)
안녕하세요 : )
드로이드나이츠에 조금이나마 기여할 수 있다는 마음으로 기쁘게 작업했습니다.
화면별로 연관성을 가지고 있는 컴포넌트들끼리 묶어 별도의 파일로 분리하였습니다. (feature/component)
해당 작업을 통해
XxxScreen()과 같은 최상위에 가까운 Composable 함수의 코드를 가볍게 만들었습니다.화면을 분리하면서 발생한 컴포넌트에 대한 Preview를 작성하였습니다.
이 과정에서
PreviewParameterProvider를 사용했습니다.이미 분리되어 있지만 Preview가 작성되어 있지 않아 어떤 구성 요소인지 파악하기 어려운 컴포넌트에 대해 Preview를 작성하였습니다.
TO-DO
feature (화면별 컴포넌트 분리)
core/designsystem (공통 컴포넌트 분리)
Preview 작성