[개선] feature 모듈별 컴포넌트 분리

tmdgh1592 commented 1 month ago

Issue

related to #308

Overview (Required)

이전 PR을 분리하고자 새롭게 PR요청드립니다. 리뷰에 남겨주신 것처럼 하나의 PR에 작업량이 많아지다보니, 기존에 있던 Preview를 이동시킨 것 외에는 추가로 Preview를 구현하지 않았습니다. 해당 PR이 merge가 가능해지면 그 이후에 분리된 컴포넌트에 대해서 Preview를 작성하겠습니다.

컴포넌트 파일 분리는 디자인 수정을 기준으로 삼았습니다. (특정 컴포넌트의 디자인이 변경되었을 때 해당 파일만 수정하면 되도록) 컴포넌트 함수 분리는 하위 컴포넌트간의 연관성을 기준으로 나누었습니다.

// before
ATopAppBar()
...
LazyColumn(...) {
    items {
        AItem()
    }
}

// after
ATopAppBar()
...
AList()

@Composable
private fun AList() {
    items {
        AItem()
    }
}

제가 놓쳤거나 더 나은 개선 방안이 있다면 수정하겠습니다. 감사합니다.

TO-DO

[x] bookmark
[x] contributor
[x] home
[x] main
[x] session
[x] setting

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 d8ad3480.

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

tmdgh1592 commented 1 month ago

@taehwandev 리뷰 남겨주셔서 감사합니다. 저 또한 태환님의 의견에도 공감하는 부분이 있습니다.

다만, 제가 작업하면서 느낀 함수 분리의 목적은 다음과 같습니다.

재사용 가능성
함수 분리로 용도 파악을 위함
이전 코드와 컨벤션 유지

1번은 태환님의 의견과 동일하지만 2, 3번이 추가되었다고 할 수 있습니다.

이전 코드를 살펴보면 단순 Text, Image를 감싼 코드지만 이미 분리되어 있는 경우가 많습니다. 저는 이런 기존의 코드 자체가 나쁘거나 틀리다고 생각하지 않고 2번 장점을 만족하는 하나의 사례가 된다고 생각합니다. 하지만 이전PR에서 남겨주신 것과 같이 과도한 분리는 해가 되기도 하겠지만요.

그리고 작업을 하다보니 이전 코드와의 컨벤션을 지키기 위해 분리한 목적도 있습니다. 처음 코드를 봤을 때 어떤 코드는 분리되어 있고, 어떤 코드는 분리되어 있지 않아 무엇을 기준 삼아 분리해야 할지 고민을 많이 했습니다.

단순 재사용을 위해서만 함수를 분리해야 한다면 기존에 Text or Image로만 분리되어 있던 코드들도 함수로 감싸지 않은 형태로 변경해야 할까요?

글이 조금 길어졌습니다,, 아직 공부하고 있는 학생이기 때문에 부족한 점이 많거나 제 의견에 틀린 부분이 있을 수 있습니다. 더 나은 코드를 작성하고자 의견을 듣고 싶습니다.

taehwandev commented 1 month ago

@tmdgh1592 저도 적고나서 기존 코드가 유사한 형태일것 같았는데요.

컴포즈를 한 3년째 사용하고있는 상황인데, 단순 Text/Image와 같은 내부 컴포넌트 구성은 효율적입니다. 아마 기존 코드에서도 보실 수 있었겠지만 기존 코드도 과도한 분리로 보여지는 부분들이 있습니다. 이걸 Diff로 한다한들 이전과 현재는 항상 동일할겁니다. 근데 저희가 하는건 스크린 단위에서 합쳐져야하고, 컴포넌트단위에서 합쳐지게됩니다. 여기에서 가장 큰 변화가 일어날거구요. 그렇다면 컴포넌트를 다시가져와 컴포넌트를 합치는 수준으로도 이미 diff 로 이전과 현재를 비교하면 충분하죠. 디자인 가이드상으로도 이게 최적일거구요.

개인적인 분리는 아래와 같습니다.

디자인 컴포넌트 - 공용 컴포넌트(공통으로 사용되어질 List 아이템이 있다면 디자인 컴포넌트에 포함합니다.)
- 어차피 UI를 디자인하고, 이 이상의 분리는 필요치 않습니다.
- 이 경우 장/단점은 하나가 수정되어지면 모든 화면에 영향을 미치게됩니다. 근데 이건 디자이너와의 협의를 통해 해결할 수 있는 부분입니다.
Screen
- 현재 화면을 하나의 스크린으로 처리합니다. 디자인 컴포넌트를 합쳐서 하나의 화면을 만들거나, 이 모듈에서만 사용하는 컴포넌트를 가져다 사용합니다.
- Screen에서의 컴포넌트 딱 요정도가 다입니다.

Text/Image를 하나하나 분리했을때의 장점이 없다는 부분이 이 부분입니다. 어차피 Text 컴포넌트는 style을 적용해서 공통화 시켜둔 상태이구요. 그래서 말씀하신 부분의 2, 3번 케이스도 충분히 이해가가지 만 분리했을때의 장점을 찾진 못했습니다. 그래서 과도한 분리라고 말씀 드린 부분입니다.

제가 나눈다면

fun Screen() {
   LazyColumn {
     items(items) { item ->
       when (item) {
           is Type -> {
                TypeComponent()
           }
           is TypeTwo -> {
                TypeTwoComponent()
           }
       }
     }
   }
}

fun TypeComponent() {
  Row {
     DroidImage()
     DroidText()
     Spacer()
     DroidText()
  }
}

fun TypeTwoComponent() {
  Row {
     DroidCheck()
     DroidText()
     Spacer()
     DroidText()
  }
}

이정도일것 같습니다. 어차피 저 리스트를 꾸며주기 위한 Component일 뿐이니 저걸 벗어나지도 않습니다. 분리한다고 했을때 저 함수를 통해서 아 이건 그냥 이 리스트를 만들기 위한 컴포넌트이구나를 파악하는것이죠.

그 안에있는 내용을 다 분리할 필요는 없는것이구요.

분리를 했다면 적절한 Preview도 필요합니다. 근데 프리뷰 상태도 저 리스트에 대한 프리뷰이지 각각의 Text()를 다시 Preview 하지 않을것 같습니다. 그걸 한다한들 장점은 없구요. 어차피 List에 포함되어진결과물이 중요한것일 뿐이구요.

tmdgh1592 commented 1 month ago

@taehwandev 답변 감사합니다! 말씀해주신 내용을 최대한 코드에 녹여내고자 불필요한 함수 분리는 최대한 제거하고 공통으로 사용될 수 있는 부분은 분리해보았습니다.

예를 들어, ContributorCard() 하나로 해결될 수 있는 코드가 ContributorItem() 함수로 감싸져서 또 분리되어 있는 부분을 수정했습니다. - 관련 커밋

또는, SponsorCard의 텍스트 스켈레톤에 대한 코드가 중복인 것으로 확인했고, 이를 분리하여 재사용하고자 TextSkeleton() 으로 분리하는 등의 작업을 했습니다. - 관련 커밋

감사합니다

droidknights / DroidKnightsApp