[개선] UI 파일을 단위별로 분리

tmdgh1592 commented 1 month ago

Issue

close #308

Overview (Required)

안녕하세요 : ) 드로이드나이츠에 조금이나마 기여할 수 있다는 마음으로 기쁘게 작업했습니다.

화면별로 연관성을 가지고 있는 컴포넌트들끼리 묶어 별도의 파일로 분리하였습니다. (feature/component) 해당 작업을 통해 XxxScreen() 과 같은 최상위에 가까운 Composable 함수의 코드를 가볍게 만들었습니다.
화면을 분리하면서 발생한 컴포넌트에 대한 Preview를 작성하였습니다. 이 과정에서 PreviewParameterProvider 를 사용했습니다.
이미 분리되어 있지만 Preview가 작성되어 있지 않아 어떤 구성 요소인지 파악하기 어려운 컴포넌트에 대해 Preview를 작성하였습니다.

TO-DO

feature (화면별 컴포넌트 분리)

[x] bookmark
[x] contributor
[x] home
[x] main
[x] session
[x] setting
core/designsystem (공통 컴포넌트 분리)
[x] RoundedImage
[x] CircularNetworkImage
[x] IconToggleButton
Preview 작성
[x] 해당 PR에서 분리하며 발생한 컴포넌트에 대한 preview 작성
[x] 이미 분리되어 있지만 preview가 없는 컴포넌트에 대한 preview 작성

github-actions[bot] commented 1 month ago

Test Results

19 tests 19 :white_check_mark: 5s :stopwatch: 11 suites 0 :zzz: 11 files 0 :x:

Results for commit 36a2ae72.

:recycle: This comment has been updated with latest results.

taehwandev commented 1 month ago

@tmdgh1592 작업해주신 내용은 정말 감사합니다. 하지만 지금의 코드 분리가 어떠한 장점이 있는지 전혀 이해가 어려운 상태입니다. 컴포넌트를 세분화해서 하나하나 분리했을때의 장점이 무엇일까요? 단순 Text가 잘 보이기 위함일까요? 개인적으론 Text, Image, Text 가 있다면 이미 Text/Image는 컴포넌트로 내부에서 사용할 부분이 분리되어있을거고, 이를 합쳐서 하나의 리스트/Box의 컴포넌트로 통합해서 정리하는 수준이면 충분해보입니다. 결국 한라인이 잘 종합되어지는 List 아이템 하나면 이미 컴포넌트로써의 가치는 충분합니다.

그리고 컴포넌트라함은 재사용이 가능해야합니다. 현재의 함수 나눈 부분은 재사용이 1도 불가능합니다. 그냥 하나의 리스트에 포함되어질 컴포넌트를 Text 단위로 분리했을 뿐이지 이걸 다른곳에서 재사용이 전혀 불가능하고, 수정을 한다고 하더라도 리스트 UI가 새로운 형태로 만들어지면 수정의 범위가 넌무 넓어지는 형태입니다.

우리가 함수를 나누었을때의 장점이 있는데, 함수를 많이 나눈다고 명확하진 않습니다. 목적이 분명해야합니다.

죄송하지만 목적에 맞는 분리를 생각하고, 분리를 해보자는 티켓을 작성한것인데요. 아쉽지만 현재 구조로는 머지가 불가능합니다.

tmdgh1592 commented 1 month ago

@taehwandev 시간내서 리뷰해주셔서 감사합니다. 사실 Text까지도 하나의 파일 내에서 함수로 분리한 이유는 함수의 이름을 보고 어떤 뷰인지 바로 파악하기 위함이었습니다. Text 하나라고 생각해볼 수도 있지만, Compose 특성상 몇 가지 설정들을 하다보면 기본으로 4~5줄이 되어 코드만으로 전체적인 뷰 구조를 파악하는데 시간이 조금 더 소요될 것이라 생각했습니다. 그러다보니 과하게 분리를 하게 된 것 같습니다 🥲

리뷰를 보고 나니 제가 생각한 장점보다 코드를 수정할 때 느낄 불편함이 조금 더 커보이네요,,

기존 코드를 바탕으로 재사용 가능성 or 기능별 분리에 대해 다시 생각해보고 리뷰요청을 드려도 될까요?

taehwandev commented 1 month ago

@tmdgh1592 보통 스크린 단위 하나, 컴포넌트 하나에서의 다양한 변화를 직접적으로 보고, 수정하는게 좋을것 같단 생각이 드네요. 길어져서 Text 하나를 분리했다곤 하지만 지금의 코드상 Title 하나, SubMessage 하나 이런식이라서 나중에 수정을 한다면 그걸 다 찾아서 수정을 해줘야 하는 단점도 있습니다.

일단 제가 원래 의도한 단위별로 분리는 이와 같았는데 충분히 설명하지 못했던것 같네요.

지금의 코드는 하나의 파일에 너무 많은 컴포넌트로 구성
이에 대한 컴포넌트화는 잘되어있지만, 파일도 어느정도 분리하면 좀 더 보기 편할것 같다

란 정도로만 생각하고 분리해보자란 글을 올렸습니다.

Text 도 하나하나 분리하면 충분히 의미가 없는건 아니겟지만, 조합으로 본다면 이미 Text는 분리되어있고, 이를 가져다 재사용하는 구조인데, 이걸 다시 구조화 하려고 분리했다면... 적절할까? Style 기준으로 분리하는것이 적절할 것인가? Style까지 구분해서 나눌 필요는 전혀 없습니다. 이미 그렇게 사용하도록 Text() 컴포넌트가 제공되어지고 있으니깐요. 조합의 의미로 본다면 이미 기본 컴포넌트들만으로도 조합해서 필요한 정보를 셋해서 사용할 수 있어보입니다. 그렇다고 Style 별로 다 구분해서 분리할 이유는 없구요. 너무 많은 함수는 결국 추가 생성을 야기합니다. theme의 스타일만 가지고도 충분히 설정이 가능한 형태라서요.

함수를 많이 나누는것도 적절한 의미가 있는것이 좋을것 같습니다. 다시 한번 고민 부탁드립니다. 필요하다면 쓰레드에 이야기 나눠도 좋을것 같구요.

tmdgh1592 commented 1 month ago

@taehwandev 말씀해주신 내용들 모두 공감합니다. 학습과 생각 정리에도 많은 도움이 되었습니다. 조금 더 고민해보면서 수정하겠습니다 🙂

tmdgh1592 commented 1 month ago

@taehwandev 안녕하세요! 말씀해주신 내용들 반영해서 Text, Image와 같은 부분들은 원복시켜두었습니다. 가능한 기능별로 묶어서 분리하려고 노력했는데 혹시 미흡한 부분이 있다면 수정해두겠습니다. 감사합니다!

+) 기존 main브랜치 코드에 SessionTitle, BookmarkImage, SessionDetailTitle 등 단순 Text, Image를 감싼 함수가 존재합니다. 리뷰에서 말씀해주신 것처럼 다음과 같은 코드 또한 함수로 분리하지 않는 구조로 변경해놓으면 좋을지 의견을 듣고 싶습니다!

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,
    )
}

tmdgh1592 commented 1 month ago

@taehwandev 리뷰 감사합니다 제가 한 PR에 너무 많은 내용을 담았네요,,

추천해주신 PR 분리 방법으로 공통 모듈에 대한 PR을 요청하고, 이후에 feature 모듈에 대한 PR을 올리겠습니다 😃

PR : 공통 모듈 -> feature 컴포넌트 분리 -> Preview 작성

+) 이전 커밋으로 돌리려고 포스 푸시를 하다가 PR이 닫혀서 위 순서대로 새로 PR을 요청하겠습니다!

droidknights / DroidKnightsApp