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

[Feature/session_bookmark]: 북마크 UI 추가 작업 #242

Merged
merged 9 commits into from
Sep 4, 2023

Conversation

kez-lab
Copy link
Contributor

@kez-lab kez-lab commented Aug 30, 2023

Issue

Overview (Required)

  • BookmarkUscase를 활용하여 Bookmarked 된 Id를 매칭하였습니다.
  • Session DTO에 isBookmarked라는 Boolean 변수를 추가하여 UIState에서 활용가능하도록 수정하였습니다.

PersistentList 를 사용하는 이유에 대해 인천 IO 세미나를 통해 너무 잘 이해했습니다, 감사합니다 🙇‍♂️

Screenshot

Before After

@github-actions
Copy link

github-actions bot commented Aug 30, 2023

Test Results

18 tests   18 ✔️  8s ⏱️
11 suites    0 💤
11 files      0

Results for commit d33ef15.

♻️ 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.

감사합니다!

지금도 충분히 좋지만 몇 가지 개선 제안이 있습니다 🙂

Column(
modifier = Modifier.padding(CardContentPadding)
) {
// 카테고리
Copy link
Member

Choose a reason for hiding this comment

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

SessionCardContent의 Depth도 늘어나고 코드 추가도 생겼는데, 이제는 주석으로 구분하기보다 의미있는 컴포저블 함수 단위로 나눌 수 있는 부분들은 분리하면 어떨까요?

이 PR의 작업과는 거리가 있는 변경사항이라 이 코멘트 반영은 선택사항으로 고려해주세요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵넵 저도 분리하는 것이 좋다고 생각합니다, 수정 고려하겠습니다.

}.catch { throwable ->
_errorFlow.emit(throwable)
}.collect { combinedUiState ->
_uiState.emit(combinedUiState)
Copy link
Member

Choose a reason for hiding this comment

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

value로 값을 초기화하면 어떨까요?

Suggested change
_uiState.emit(combinedUiState)
_uiState.value = combinedUiState

emit은 suspend 함수이고 내부에서 value로 값을 세팅합니다. 지금 상황에서는 꼭 suspend 함수를 사용할 필요는 없어보입니다.

    override suspend fun emit(value: T) {
        this.value = value
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앗 그렇군요 suspend 함수를 쓰는 기준이 궁굼합니다..!!
제가 Flow에 대한 이해가 깊지 못해서 errorFlow를 방출하는 부분과 동일하게 작성했는데 두 Flow의 value는 다르게 방출되어야 하는 것일까요??

Copy link
Member

Choose a reason for hiding this comment

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

제가 Flow에 대한 이해가 깊지 못해서 errorFlow를 방출하는 부분과 동일하게 작성했는데 두 Flow의 value는 다르게 방출되어야 하는 것일까요??

errorFlow는 SharedFlow 타입이기 때문에 현재 상태를 읽고 쓸 수 있는 StateFlow와 다릅니다. SharedFlow에는 value가 없습니다.
https://developer.android.com/kotlin/flow/stateflow-and-sharedflow?hl=ko

suspend 함수를 쓰는 기준이 궁굼합니다..!!

사실 이 상황에서 suspend 함수를 사용한다고 해서 크게 문제가 되는건 아닙니다! 다만 MutableStateFlow의 값을 세팅하는데에 굳이 suspend를 거칠 필요가 없기 때문에 제안드린것이였어요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아하 이해되었습니다 감사합니다!!

@wisemuji
Copy link
Member

인천 I/O Extended에 오셨었군요! 반갑습니다 😊

Comment on lines 143 to 153
Image(
painter = painterResource(id = R.drawable.ic_flagbookmark),
contentDescription = null,
modifier = Modifier.Companion
.align(Alignment.TopEnd)
.padding(end = 30.dp)
.size(
width = 24.dp,
height = 36.dp
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

북마크 이미지가 오른쪽에 배치되는 것은 이 컴포저블을 호출한 곳에서 지정해 주면 어떨까요?
만약 북마크 이미지를 왼쪽에서도 놓을 수 있고 오른쪽에서도 놓을 수 있다면 현재 구조에서는 수정이 어렵습니다.

Modifier를 파라미터로 넘기는 방법을 참고 해보세요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

거기까지는 생각하지 못했습니다..!!
유연한 코드를 작성하도록 수정하겠습니다

initialValue = SessionUiState.Loading
)
private val _uiState = MutableStateFlow<SessionUiState>(SessionUiState.Loading)
val uiState: StateFlow<SessionUiState> get() = _uiState
Copy link
Contributor

Choose a reason for hiding this comment

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

MutableStateFlow를 StateFlow로 변환할 때에는 asStateFlow()를 이용해 ReadOnly 타입으로 변경합니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 수정하겠습니다!

@kez-lab
Copy link
Contributor Author

kez-lab commented Sep 2, 2023

@wisemuji @laco-dev
코드리뷰 덕분에 많이 배웠습니다 감사합니다 🍀

해당 부분 반영하여 수정하였습니다.
시간 되실 때 확인해주시면 감사하겠습니다.

Copy link
Contributor

@laco-dev laco-dev left a comment

Choose a reason for hiding this comment

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

피드백 반영 감사합니다.

@wisemuji
이상 없으시면 병합 부탁드립니다.

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.

확인이 늦었네요...!
리뷰 반영 고생 많으셨습니다 👏

@wisemuji wisemuji merged commit 4b4c095 into droidknights:main Sep 4, 2023
2 checks passed
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.

[세션, 북마크] 세션 목록에서 북마크 여부가 노출된다
5 participants