Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[개선] 공통 모듈에서 사용할 수 있는 UI 파일 분리 #326

Closed
wants to merge 6 commits into from

Conversation

tmdgh1592
Copy link
Contributor

@tmdgh1592 tmdgh1592 commented Jun 5, 2024

Issue

Overview (Required)

이전 PR을 분리하고자 새롭게 PR요청드립니다.
해당 PR에서 작업한 내용은 크게 2가지입니다.

IconToggleButton

IconToggleButton을 사용하기 위해서 중첩된 구조가 되어야 하는 부분을 개선하기 위해,
커스텀 IconToggleButton으로 감싸서 사용성과 가독성을 높였습니다.

기존 IconToggleButton을 사용하기 위해서는 다음과 같은 구조였습니다.

// before
IconToggleButton(
    checked = checked,
    onCheckedChange = onCheckedChange,
) {
    Icon(
        painter =
        if (bookmarked) {
            painterResource(id = checked_image)
        } else {
            painterResource(id = unchecked_image)
        },
        contentDescription = null,
        tint = if (checked) Purple01 else Gray
    )
}

// after
IconToggleButton(
    isChecked = checked,
    onCheckedChange = onCheckedChange,
    checkedImageRes = checked_image,
    uncheckedImageRes = unchecked_image,
)

RoundedImage

모서리가 둥근 이미지를 만들기 위해 매번 Surface로 감싸야 하는 번거로움을 줄이고자 RoundedImage로 분리하였습니다.
imageSize, roundSize, border를 전달받을 수 있으며, imageSize를 별도로 전달하지 않을 경우 aspectRatio를 1:1 비율로 적용하도록 구현하였습니다.

// before
Surface(
    shape = RoundedCornerShape(16.dp),
    onClick = onClick,
) {
    Image(
        painter = painterResource(id = imageRes),
        contentDescription = null,
        modifier = Modifier.aspectRatio(1f)
    )
}

// after
RoundedImage(
    imageRes = imageRes,
    onClick = onClick,
)

or 

RoundedImage(
    imageRes = imageRes,
    border = BorderStroke(12.dp, Color.Green),
    onClick = onClick,
)

Links

Copy link

github-actions bot commented Jun 5, 2024

Test Results

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

Results for commit 1e07d29.

♻️ This comment has been updated with latest results.

contentDescription = null
)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

width가 null이고 rounded 사이즈가 있는 상태인데 함수가 2개일 필요가 있나요? 나머지 내용은 모두 동일한 상태라서
아래꺼 하나로 충분해보입니다.

그리고 이 버튼은 ImageButton을 뜻하는것 같은데 ... 용도와 타이틀도 안맞는것 같네요. 아마 이런 형태가 main 쪽에만 있었는데 공통화 하시면서 그대로 이동한것 같은데. 함수명으로는 click 가능한 이미지 인지이해가 어렵네요.
그리고 사이즈를 바꾸면 Image가 센터로 정렬되지 않습니다.

contentDescription = null
)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

마지막 줄 엔터가 저희 코드 컨벤션입니다.

  • 해당 컴포넌트에 대한 Preview 추가 부탁드려요.

Copy link
Member

@taehwandev taehwandev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존 코드 그대로이긴 하지만. 개선을 위해서 코멘트 드린 부분입니다.
공통 컴포넌트화를 시킨 부분이라 좀 더 개선을 위함이니 이해부탁드려요.
내부에만 있다면 원래 있던 형태로 사용해도 문제가 없습니다.

@tmdgh1592
Copy link
Contributor Author

@taehwandev 리뷰 감사합니다!
말씀해주신 것처럼 위 두 가지 컴포넌트는 우선은 한 곳에서만 사용되기도 하고,
공통 컴포넌트화 하기 애매한 부분이 있어서 원복시켜놓겠습니다.


이 외에 designsystem의 BottomLogo.kt에 2023으로 되어 있는 부분이 있습니다.
반면, 세션 화면에는 BottomLogo를 사용하지 않고 비슷하지만 별도로 함수가 만들어져 있어 2024으로 표시되고 있습니다.

2023년도가 의도된 것인지, 공통 컴포넌트를 사용하지 않아 불일치가 발생한 것인지 여부를 몰라서 따로 수정하지 않았는데,
말씀해주신다면 세션 화면에 있는 Footer도 공통 컴포넌트를 사용하도록 변경해놓겠습니다.

이 부분이 해당 PR에서 다루기 적합하지 않다고 생각되신다면, 변경사항이 없기 때문에 PR을 닫아주셔도 좋을 것 같습니다 🙇🙇

// BottomLogo.kt
@Composable
fun BottomLogo(color: Color = LightGray) {
    Box(
        modifier = Modifier
            .fillMaxWidth()
            .height(BottomLogoHeight),
        contentAlignment = Alignment.Center
    ) {
        Text(
            text = "Droid Knights 2023",
            style = KnightsTheme.typography.labelMediumR,
            color = color,
        )
    }
}

// SessionScreen.kt
@Composable
private fun DroidKnightsFooter() {
    Text(
        modifier = Modifier
            .fillMaxWidth()
            .wrapContentHeight()
            .padding(top = 56.dp, bottom = 80.dp),
        text = stringResource(id = R.string.footer_text),
        style = KnightsTheme.typography.labelMediumR,
        color = Color.LightGray,
        textAlign = TextAlign.Center
    )
}

스크린샷 2024-06-05 오후 9 39 12
스크린샷 2024-06-05 오후 9 40 00

@taehwandev
Copy link
Member

올려주신 내용은 별도의 내용으로 보이네요. 클로즈 하겠습니다.

@taehwandev taehwandev closed this Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants