Skip to content

[Feature/#410] 세션 상세 페이지 구현#484

Merged
workspace merged 22 commits into
droidknights:2025/compose-multiplatformfrom
angryPodo:feature/sessiont-detail-page
Jun 6, 2025
Merged

[Feature/#410] 세션 상세 페이지 구현#484
workspace merged 22 commits into
droidknights:2025/compose-multiplatformfrom
angryPodo:feature/sessiont-detail-page

Conversation

@angryPodo

Copy link
Copy Markdown

Issue

Overview (Required)

세션 상세 화면(SessionDetailScreen)의 UI 개선 및 북마크 기능 구현

  • 세션 제목, 카테고리 칩, 세션 소개, 발표자 정보를 포함한 상세 화면 레이아웃 구현
  • 북마크 기능 추가: TopAppBar에 북마크 아이콘 추가 및 토글 기능 구현
  • 북마크 상태 팝업: 북마크 저장/해제 시 애니메이션 팝업 추가
  • 세션 정보 표시: 세션 제목, 시간, 트랙, 카테고리 태그를 칩 형태로 구성
  • 발표자 정보 섹션: 세션에 참여하는 발표자 컴포넌트 구현
  • 로딩 상태 처리: 세션 데이터 로딩 중 CircularProgressIndicator 표시

Screenshot

LightMode.mp4
DarkMode.mp4

To Reviewer

첫 오픈소스 기여인만큼 부족한 점이 많습니다 😢
잘 부탁드립니다!

bookmarked = (uiState as? SessionDetailUiState.Success)?.bookmarked == true,
onClickBookmark = { viewModel.toggleBookmark() },
onBackClick = onBackClick,
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

scroll 하면 TopAppBar도 함께 스크롤 될것같은데 맞을까요?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

네 맞습니다. 혹시 스크롤시 탑바는 고정일까요?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@angryPodo 네 탑바 아래 영역만 스크롤 되도록 변경해주세요! (이전 구현도 전체 스크롤이 먹여져있네요) SessionDetailContent로 옮기면 될 것 같네요

val uiState = _uiState.asStateFlow()

private val _sessionUiEffect = MutableStateFlow<SessionDetailEffect>(SessionDetailEffect.Idle)
val sessionUiEffect = _sessionUiEffect.asStateFlow()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SideEffect를 StateFlow로 하면 화면을 벗어나 돌아오면 다시 뜨지 않을까요?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

맞네요! SharedFlow로 수정하겠습니다

_sessionUiEffect.value =
SessionDetailEffect.ShowToastForBookmarkState(newBookmarkState)
}.onFailure { throwable ->
_uiState.value = uiState.copy(bookmarked = !newBookmarkState)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

이 흐름이라면 성공했을때 값이 바뀌는게 더 맞지 않을까요?
성공했는데 통신의 오류로 이전으로 돌아오는것도 이상할 수 있고요.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

낙관적 UI를 적용한거긴합니다만 실패시 롤백 처리가 미흡하긴하네요. 지금 구조대로 가되 실패시의 토스트를 띄우는건 어떻게 생각하시나요?

@angryPodo angryPodo requested a review from taehwandev June 5, 2025 05:25
@angryPodo angryPodo requested a review from l2hyunwoo June 5, 2025 18:42
bookmarked = (uiState as? SessionDetailUiState.Success)?.bookmarked == true,
onClickBookmark = { viewModel.toggleBookmark() },
onBackClick = onBackClick,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@angryPodo 네 탑바 아래 영역만 스크롤 되도록 변경해주세요! (이전 구현도 전체 스크롤이 먹여져있네요) SessionDetailContent로 옮기면 될 것 같네요

Text(
modifier = Modifier
.padding(top = 8.dp)
.padding(end = 58.dp),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@angryPodo 이전 구현과 피그마를 그대로 설정하신 것 같은데, horizontal padding 없이 width는 그냥 fillMaxWidth로 해주세요!

import org.jetbrains.compose.resources.stringResource

@Composable
internal fun SessionDetailBookmarkStatePopup(bookmarked: Boolean) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@angryPodo
BoxScope를 제공하여, AnimatedVisibility를 이 컴포저블 함수 내부 동작으로 만들어보는 것도 좋아보입니다.

SessionDetailScreen

image

SessionDetailBookmarkStatePopup

image

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

좋은 아이디어 감사합니다 👍 👍 👍

Comment on lines +38 to +39
color = KnightsTheme.colorScheme.darkSurface,
contentColor = KnightsTheme.colorScheme.onSecondarySurface,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@angryPodo 피그마에 다크모드에서의 색상이 지정이 안되어있네요. 요거 contentColor는 그냥 Color.BLUE_01로 바꿔주세요.


@Composable
internal fun SessionDetailBookmarkStatePopup(bookmarked: Boolean) {
val messageStringRes by rememberUpdatedState(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@angryPodo rememberUpdatedState가 크게 의미가 있을까요?!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

bookmarked 파라미터가 변경될 때마다 컴포저블이 리컴포지션되기 때문에, 단순하게 가는게 좋은것 같아요


@Composable
internal fun SessionDetailChips(session: Session) {
Row(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@angryPodo 요렇게 가로가 부족하면 칩에 줄넘김이 생기는데요,

  1. Row -> FlowRow로 변경해주세요. 이 경우, verticalArrangement를 6.dp로 추가하여 행간 거리를 유지합시다!
  2. Chip 컴포넌트의 Text에 maxLines = 1을 추가해주세요. (text를 1줄로 렌더링 하기 위해 가로 길이가 모자란 경우 줄이 넘어가도록)
image

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

이 부분 미쳐 고려하지 못했네요😣 적용하겠습니다!

.clip(CircleShape),
)

Spacer(Modifier.height(16.dp))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@angryPodo figma에 12로 줄어있는데 확인 부탁드립니다. (아래 name과 introduction 사이도 12)

Text(
text = stringResource(resource = Res.string.session_detail_speaker),
style = KnightsTheme.typography.labelSmallM,
color = KnightsTheme.colorScheme.onSurface,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@angryPodo 이거 Text나 Icon마다 color를 지정하실 필요없습니다. SessionDetailScreen에 Surface를 추가하면 하위 컴포저블에 LocalContentColor를 통해 Surface에 맞는 콘텐츠 색상이 전달되며, Text, Icon에서 기본 값으로 LocalContentColor를 설정하고 있습니다. SessionScreen를 참고하세요.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

정말 좋은 방식이네요!! 관련 코드들 수정했습니다 :)

@workspace

Copy link
Copy Markdown
Contributor

@angryPodo 아! 그리고, SessionDetailScreen의 프리뷰가 하나 있으면 좋을 것 같은데요, 조금 전에 2025 세션 정보를 업데이트하여 ㅎㅎ 아래 링크의 박상권님 세션 데이터로 프리뷰를 추가해주실 수 있을까요? (가장 정보가 긴 세션)

https://github.com/droidknights/DroidKnightsApp/blob/2025/app/assets/sessions/sessions.json#L87

@angryPodo

Copy link
Copy Markdown
Author

@workspace 코멘트 해주신 부분 반영해 적용했습니다! 프리뷰가 좀 방대하긴하네요😅

@workspace workspace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@angryPodo check 모두 돌면 머지하도록 하겠습니다. 수고하셨습니다!

@workspace workspace merged commit 53e07e9 into droidknights:2025/compose-multiplatform Jun 6, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants