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

[tmdgh1592]

Issue

• related to [개선] UI 파일을 단위별로 분리해봅니다. #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

• bookmark
• contributor
• home
• main
• session
• setting

[github-actions]

Test Results

19 tests   19 ✅  5s ⏱️
11 suites   0 💤
11 files     0 ❌

Results for commit d8ad348.

♻️ This comment has been updated with latest results.

[taehwandev]

이 함수를 분리할 이유가있나요?
어차피 이 부분을 테스트 하려면 LazyColumn이 필요하고, BookmarkItemUiState를 필요하기에 재사용이 불가능합니다.

[tmdgh1592]

사실 이 부분은 반드시 분리할 필요는 없는 코드입니다.
다만 SessionScreen.kt 파일의 LazyListScope.sessionItems() 함수를 레퍼런스 삼아 코드 통일성을 유지하려고 하다 보니 분리했었습니다.
함수로 분리하지 않도록 수정해놓겠습니다!

[taehwandev]

이전에 커멘트 드렸듯이. 개인적인 생각으론 의미 없는 분리가 너무 많은것 같습니다.
함수의 분리는 단순히 함수를 분리하는것이 아닌 재사용이 가능한가를 함께 판단하여 분리합니다.

함수의 길이를 줄이자가 아니라 함수를 분리했을때 올 수 있는 장점이 있는가를 보는것이죠.

이 코드는 TagChip은 적절한 분리입니다. 2번 이상 사용하고있으니깐요. 하지만 forEach를 별도로하기 위한 분리는 큰 의미가 없어보입니다.
SectionChips를 어차피 구성하기 위한 함수일 뿐이니깐요.

@Composable
internal fun SessionChips(session: Session) {
    Row(
        horizontalArrangement = Arrangement.spacedBy(8.dp),
        verticalAlignment = Alignment.CenterVertically,
    ) {
        TrackChip(room = session.room)
        TimeChip(dateTime = session.startTime)
        TagChips(tags = session.tags.toPersistentList())
    }
}

@Composable
internal fun TagChips(tags: PersistentList<Tag>) {
    tags.forEach { tag ->
        TagChip(tag = tag)
    }
}

@Composable
internal fun TagChip(tag: Tag) {
    TextChip(
        text = tag.name,
        containerColor = DarkGray,
        labelColor = LightGray,
    )
}

유사한 형태가 몇몇 더 보여서 일단 리뷰 진행은 더하지 않았습니다. 다시 고민 부탁드려요.

[tmdgh1592]

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

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

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

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

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

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

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

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

[taehwandev]

@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]

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

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

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

감사합니다

[taehwandev]

고생하셨습니다.