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

[Session] 세션 상세 - 발표 개요 #217

Merged
merged 6 commits into from
Aug 19, 2023

Conversation

JeonK1
Copy link
Contributor

@JeonK1 JeonK1 commented Aug 17, 2023

Issue

Overview (Required)

  • 발표 개요 내용이 존재하면 해당 텍스트를 보여줍니다.
  • 원활한 코드리뷰를 위해 SessionDetailScreen 파일에 속하는 Composable 들에 대해 @Preview 를 추가하였습니다.

Screenshot

Before After

@github-actions
Copy link

github-actions bot commented Aug 17, 2023

Test Results

13 tests   13 ✔️  6s ⏱️
  9 suites    0 💤
  9 files      0

Results for commit 6acfb6f.

♻️ This comment has been updated with latest results.

Copy link
Member

@wisemuji wisemuji left a comment

Choose a reason for hiding this comment

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

발표 개요 내용이 존재하는지 분기 처리까지 해주셨군요 👍
몇 가지 코멘트 확인 부탁드립니다.

Comment on lines 76 to 82
@Preview
@Composable
private fun SessionDetailTopAppBarPreview() {
KnightsTheme {
SessionDetailTopAppBar { }
}
}
Copy link
Member

@wisemuji wisemuji Aug 17, 2023

Choose a reason for hiding this comment

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

Preview 함수들은 파일 하단에 모아두면 어떨까요?
개발자가 위에서부터 UI 코드 문맥을 파악할 때 관심사를 가질 UI 로직이 위에 밀집되어 있는게 좋아보입니다.

Copy link
Member

Choose a reason for hiding this comment

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

개인적으로 함수의 순서는
ViewModel을 전달 받는 함수 하나(State를 전달 받을 수도 있을거구요.

@Composable
fun 외부에서 보여지는 함수() {}

@Composable
private/internal fun 프리뷰를 위한 함수() {}

@Preview
@Composable
internal fun Preview에 함수이름() {}

요런식으로 구반하고 사용하고 있습니다.

위치는 프리뷰가 가장 아래이고요. 아무래도 프리뷰를 통해 이것저것 UI 확인이 가능하기도하니 테스트 코드도 포함될수도 있고요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

확실히 테스트코드 영역을 별도로 분리하니 코드리뷰시에 불필요한 테스트코드를 확인하는 일을 줄일 수 있어서 더욱 좋아보입니다.
코멘트 감사합니다. 반영하였습니다. 4f8325d

Comment on lines +109 to +115
if (session.content.isNotEmpty()) {
Spacer(modifier = Modifier.height(16.dp))
SessionOverview(content = session.content)
Spacer(modifier = Modifier.height(56.dp))
} else {
Spacer(modifier = Modifier.height(40.dp))
}
Copy link
Member

Choose a reason for hiding this comment

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

(선택사항)
세션 데이터에 따라 다른 UI가 보여지는 것을 가시적으로 Preview에서 확인하고 싶다면 PreviewParameter를 활용해볼 수 있습니다. Preview를 많이 정의해주셔서 제안드렸으나 꼭 반영하진 않으셔도 됩니다 🙂

https://developer.android.com/jetpack/compose/tooling/previews#preview-data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Preview 코드 작성하면서 개요 데이터가 존재할 때와 존재하지 않을때의 Preview를 각각 작성해야할지 하나하나 작성하면 코드가 길어질 것 같으니 안해야할지 고민하다 대표 값 하나만 작성했었는데요..!
덕분에 PreviewParameter 를 적용하여 이러한 걱정들을 해소할 수 있었습니다.
감사합니다. 반영하였습니다! a16a753

Copy link
Member

@wisemuji wisemuji left a comment

Choose a reason for hiding this comment

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

코멘트 잘 반영해주셨네요 🙇
오타는 PR에서 바로 커밋했습니다.

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.

[Session] 세션 상세 - 발표 개요
4 participants