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] 수집 도감 UI 및 기능 구현 #115

Merged
merged 15 commits into from
Feb 28, 2024
Merged

Conversation

ham2174
Copy link
Collaborator

@ham2174 ham2174 commented Feb 27, 2024

관련 이슈

작업한 내용

  • 수집 도감 페이지 UI 제작
  • 수집 도감 기능 구현
    • 이제 매칭한 사용자의 MBTI가 수집도감에 새롭게 추가됩니다.

PR 포인트

  • MBTI 뱃지 36종(active 16종, inactive 16종) 추가
  • 트로필 아이콘 추가
    image

🚀Next Feature

  • v1.2.0 배포 준비

@ham2174 ham2174 added ham😸 Good for newcomers Feat labels Feb 27, 2024
@ham2174 ham2174 requested a review from murjune February 27, 2024 18:09
@ham2174 ham2174 self-assigned this Feb 27, 2024
@ham2174
Copy link
Collaborator Author

ham2174 commented Feb 27, 2024

@murjune 파일 추가가 상당히 많으므로 꼭 한번 확인해주었으면 하는 부분 남길게..!

Copy link
Collaborator Author

@ham2174 ham2174 left a comment

Choose a reason for hiding this comment

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

요렇게 한번 봐주시면 감사드립니다요~~ @murjune

Copy link
Member

@murjune murjune left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! comment 확인해주세욥~

@@ -57,6 +57,7 @@ dependencies {
implementation(projects.feature.home)
implementation(projects.feature.match)
implementation(projects.feature.onboarding)
implementation(projects.feature.collection)
Copy link
Member

Choose a reason for hiding this comment

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

collection 을 수집 한다는 의미로 사용한걸까??
만약, 나중에 뱃지, mbti 등등 모든걸 이 모듈에서 관리하게 될거 같은데 오히려 모듈의 복잡도가 너무 올라갈 수 있겠다는 생각이드네유

하지만, 일단은 수정하지 않고 진행하쥬~~ 구현이 먼저니까 👍

Comment on lines 53 to 62
override suspend fun fetchUserMbtiCollection(): Flow<List<Mbti>> = flow {
emit(
value = userDataStore.mbtiCollection.map { mbti ->
Mbti.valueOf(mbti)
}
)
}.catch {
emit(emptyList())
Timber.e(it.message)
}
Copy link
Member

Choose a reason for hiding this comment

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

이 부분을 flow 로 한 이유가 있을까??
emit 을 한 곳에서만 하고 있어서 Flow로 만들 이유가 없어보여!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Flow를 사용하면 어떤 장점이 있을까 생각하면서 작업을 하다가 코드리뷰를 한번 받아보고 싶었어! 구현해보니까 현재로써는 Result를 사용해서 처리한것과 크게 다른점이 없어보이기도 하고! 한가지 궁금한점이 Flow를 사용해서 데이터를 원샷으로 보냈을때와 Result를 사용하는것과 어떤 차이가 있을까?

Copy link
Member

Choose a reason for hiding this comment

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

일단 Result와 Flow는 비교대상이 아니야!

Result는 어떠한 일련의 로직에 대한 에러핸들링을 도와주는 kotlin의 api야
하지만 코루틴 Flow같은 경우는 실시간 데이터를 비동기적으로 반응형으로 처리하기 위한 api지.

따라서, 둘을 비교할수 없음!! 순수 코루틴과 Flow를 비교한다면, 실시간 데이터 처리가 매우 중요한경우(e. 채팅)에 사용할 수 있겠지!

리액트 자바와 비슷한 애라고 보면돼!

Copy link
Member

Choose a reason for hiding this comment

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

추가적으로 flow를 쓴다면 suspend 키워드는 의미가 없습니당

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아하 이해됐어! 그러면 확실하게 지금 상황에서 Flow를 사용하는건 어울리는 사용법은 아니라는게 맞는것같네!! Result로 바꾸고 멘션한번 남길게! 오늘 업데이트까지 올리는걸로 갑니다욧...!

Comment on lines +28 to +31
private fun saveMbtiCollection(mbti: String) {
dataStore.mbtiCollection = dataStore.mbtiCollection.toMutableSet().apply {
add(mbti)
}.toSet()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private fun saveMbtiCollection(mbti: String) {
dataStore.mbtiCollection = dataStore.mbtiCollection.toMutableSet().apply {
add(mbti)
}.toSet()
private fun saveMbtiCollection(mbti: String) {
dataStore.mbtiCollection = dataStore.mbtiCollection + mbti

plus opeerator를 사용하는게 더 간결할 거 같네용

@ham2174 ham2174 merged commit e659c76 into develop Feb 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feat ham😸 Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] 수집 도감 기능
2 participants