Skip to content

[#511] Bookmark 페이지 추가#523

Merged
workspace merged 19 commits into
droidknights:2025/compose-multiplatformfrom
easternkite:feature/#511-bookmark_screen
Jun 14, 2025
Merged

[#511] Bookmark 페이지 추가#523
workspace merged 19 commits into
droidknights:2025/compose-multiplatformfrom
easternkite:feature/#511-bookmark_screen

Conversation

@easternkite

@easternkite easternkite commented Jun 9, 2025

Copy link
Copy Markdown

Issue

Overview (Required)

  • CMP 앱에 북마크 페이지를 추가하였습니다. (2025/app 브랜치 코드 기반으로 작성하였습니다.)
  • 필요한 colorScheme은 추가하였습니다.
  • 컴포넌트마다 Dark, Light 프리뷰를 추가하였습니다.
  • 안드로이드 Target 한정으로 BackHandler가 필요하기 때문에, nonAndroidTarget을 추가하여 BackHandler를 구현하였습니다.
  • 스낵바 연결이 필요한 부분은 TODO 주석으로 남겨놓았습니다.

Fix

  • GetBookmarkedSessionsUseCaseImpl의 의존항목 중에 인터페이스가 아닌, 실구현 클래스를 참조하여 Koin 에러가 나는 케이스를 수정하였습니다. 462fd7f

Screenshot

image


@Composable
fun BookmarkScreen(
fun BookmarkRoute(

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.

여긴 screen이 맞는것 같은데 수정하신 이유가

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.

@taehwandev
2025/app 브랜치 버전과 네이밍을 일치시키기 위함이었습니다~

해당 브랜치 기준으로 요 함수만 BookmarkScreen으로 바꿀까도 생각했었는데요, 이렇게되면 BookmarkScreen이 두개가 되어서, 기존대로 Route 네이밍을 사용하였습니다.

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.

네! 확인 감사합니다.

) {
items(
items = bookmarkItems,
key = { item -> item.session.id },

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.

contentType 추가해주시면 최적화에 더 좋습니다

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.

@taehwandev
북마크 리스트의 모든 아이템이 모두 동일한 ViewType(레이아웃 구조)로 보이는데요,
이 경우에도 contentType을 지정하는 것이 성능에 의미가 있는지 궁금합니다.

_errorFlow.emit(throwable)
}.launchIn(viewModelScope)
}
}

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.

Flow로 할 필요가 있나요?

@easternkite easternkite Jun 10, 2025

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.

@taehwandev
저도 이부분 의문이긴 했었는데요~
해당 Flow Builder를 통해 만들어진 Flow를 사용하지 않는 시점에서, 해당 코드는 오버헤드라 생각합니다ㅎㅎ
기존 코드베이스를 기반으로 빠르게 Feature를 따다 보니 큰 리스크가 없다고 판단하여 적용했던 코드였습니다.

요건 try-catch나 runCatching으로 개선해볼게요~!

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.

@taehwandev 이부분 반영하였습니다. 감사합니다~ bf725ec

북마크 삭제 시 `flow`를 생성하여 예외를 처리하던 방식에서 `runCatching`을 사용하도록 수정했습니다.
이를 통해 코드를 간소화하고 가독성을 향상시켰습니다.
@workspace

Copy link
Copy Markdown
Contributor

@easternkite 안녕하세요. 현생 이슈로 리뷰가 늦어지고 있는데, 내일 중 리뷰 완료 하겠습니다🙏

@easternkite

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.

@easternkite 수고하셨습니다! ㅎㅎ 기존 코드를 kotlin multiplatform 코드로 옮기는 측면에서 살펴봤는데 너무 잘 작업해주셨습니다. 머지하겠습니다!

@workspace workspace merged commit f2cdcdb into droidknights:2025/compose-multiplatform Jun 14, 2025
9 checks passed
@easternkite

Copy link
Copy Markdown
Author

@easternkite 수고하셨습니다! ㅎㅎ 기존 코드를 kotlin multiplatform 코드로 옮기는 측면에서 살펴봤는데 너무 잘 작업해주셨습니다. 머지하겠습니다!

@workspace
바쁘셨을텐데 시간내서 리뷰해주셔서 감사합니다 🥹

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.

3 participants