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/#48 프로필 화면 구현 #139

Merged
merged 47 commits into from
Aug 1, 2023

Conversation

ki960213
Copy link
Collaborator

#️⃣연관된 이슈

#48

📝작업 내용

이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
내 프로필 화면 구현

스크린샷 (선택)

image

예상 소요 시간 및 실제 소요 시간

이슈의 예상 소요 시간과 실제 소요된 시간을 분 or 시간 or 일 단위로 작성해주세요.

예상시간/걸린시간 => 1일/2일

💬리뷰 요구사항(선택)

리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
데이터 모델이 애매해서 일단 클래스 명을 Activity1, Member1 이렇게 만들었습니다.

- 회원의 활동 정보 외의 정보를 보여주는 레이아웃 구현
- 회원의 이름, 사진, 소개글, 직무 정보를 보여주는 기능 구현
- jobs -> myprofile_jobs
- activities -> myprofile_activities
- MemberApiModel1 -> MemberWithoutActivitiesApiModel
@ki960213 ki960213 self-assigned this Jul 30, 2023
@ki960213 ki960213 added Android 안드로이드 관련 이슈 기능 추가 새로운 기능 추가 및 기존 기능 변경 High Priority 리뷰 우선순위가 높은 PR labels Jul 30, 2023
@ki960213 ki960213 added this to the 3차 스프린트 1주차 milestone Jul 30, 2023
Copy link
Member

@tmdgh1592 tmdgh1592 left a comment

Choose a reason for hiding this comment

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

프로필 화면 구현하시느라 고생하셨습니다 👍

companion object {
private val instance = ActivitiesAdapterDecoration()

fun getInstance() = instance
Copy link
Member

Choose a reason for hiding this comment

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

함수 시그너처로 반환 타입을 작성해주시면 좋을 것 같습니다 : )

Copy link
Collaborator

Choose a reason for hiding this comment

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

getInstance 함수를 만든 이유가 있을까요...? 저는 instance 의 pirvate 을 빼고 아래 함수를 지우는 것이 좋아보입니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

instance를 쓸거면 object로 만드는 게 더 낫겠죠?

Copy link
Member

Choose a reason for hiding this comment

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

아! 스캇 리뷰를 보니까 의문이 생겼습니다.
단순 ItemDecoration까지 싱글톤으로 관리하시려는 이유가 있으신가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

여러 리사이클러 뷰에 같은 ItemDecoration 인스턴스를 설정하기 위함이었습니다.
생각해보니 싱글톤이 아니라 그냥 ItemDecoration을 생성하고 생성된 인스턴스를 설정해주면 되는 거였네요!

Comment on lines 6 to 17
data class MyProfileScreenUiState(
val isLoading: Boolean,
val isError: Boolean,
val errorMessage: String,
val memberId: Long,
val memberName: String,
val description: String,
val memberImageUrl: String,
val jobs: List<ActivityUiState>,
val educations: List<ActivityUiState>,
val clubs: List<ActivityUiState>,
val events: List<ActivityUiState>
Copy link
Member

Choose a reason for hiding this comment

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

프로퍼티가 엄청 많군요 😱

Copy link
Collaborator Author

@ki960213 ki960213 Jul 31, 2023

Choose a reason for hiding this comment

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

로딩, 에러, 성공 시 필요한 프로퍼티가 다르긴 하지만, 데이터 바인딩을 할 땐 위와 같은 방법이 더 편하기 때문에 위의 방법을 선택했습니다 😢
UI State Modeling 패턴 중 데이터 바인딩을 사용하기에 가장 편한 "하나의 State data class를 만들어서 관리하기"를 선택했습니다.

android:layout_height="wrap_content">

<ImageView
android:id="@+id/iv_myprofile_activitynamedecorator"
Copy link
Member

Choose a reason for hiding this comment

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

이 부분은 팀 내에서 정한 컨벤션이지만, 언더바(_)가 없으니 확실히 읽기 힘들어지네요 🥲
이후에 컨벤션 수정이 시급해보입니다..!! 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저도 처음엔 위와 같은 네이밍이 단어 구분이 힘들어서 불편하다고 생각했는데, 수많은 파일이 모여있는 drawable 패키지를 볼 때 [what_where_description] 패턴이 아니라, 단어마다 언더바로 구분한다면 파일명이 모여있을 땐 보기 불편해질 것 같습니다.

Copy link
Member

Choose a reason for hiding this comment

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

저는 그런 상황에서도 불편하다고 생각하는데, 역시 사람마다 생각하는 것이 다르군요 🤔

- operator fun get(activityType: ActivityType) -> fun
getActivities(activityType: ActivityType)
…del 명 변경

- MemberActivitiesBindActivityTypeApiModel ->
ActivitiesAssociatedByActivityType
- MemberActivitiesApiModel -> ActivityApiModel
- bnvMain -> mainBottomNavigationView
@@ -0,0 +1,7 @@
package com.emmsale.data.activity

data class Activity1(
Copy link
Collaborator

Choose a reason for hiding this comment

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

지금은 DataModel 을 이렇게 Activity1 과 Acitivty 이런 방식으로 두지만, 추후에 시간이 여유롭다면 Acitivity1 하나만 있는게 좋을 것 같습니다. 나중에Onboarding 을 Activity1 의 데이터 모델을 이용하여 해결했으면 좋겠네요.

Copy link
Member

Choose a reason for hiding this comment

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

커디 프로젝트,,, 빠른 리팩토링이 시급합니다..

)

ApiSuccess(
Member1(
Copy link
Collaborator

Choose a reason for hiding this comment

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

이건 모델 클래스에서 책임지도록 하면 좋을 것 같습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

어떤 모델 클래스에서 책임지는 게 좋다고 생각하시나요?
우선 서버의 응답값을 데이터 모델로 생성하는 작업은 데이터 모델이 할 일은 아니라고 생각합니다. 비즈니스 로직이 아니기 때문입니다.
그리고 데이터 모델을 만드는 데 두 종류의 ApiModel이 필요합니다. 그래서 이 작업을 둘 중 하나에 맡긴다면 둘 중 하나는 다른 응답값에 의존하기 때문에 좋지 않다고 생각합니다. 두 ApiModel은 서로 다른 서버와의 통신에 필요하기 때문에 하나의 통신이 다른 통신에 의존하는 형태는 좋지 않다고 생각합니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

아 그렇네요 제가 착각했습니다 ㅎㅎ 아니면 private 함수로 따로 뺴도 좋을 것 같아요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

아니면

val member = memberApiModel.toData()
val activities = activitiesApiModel.associate { it.getActivityType() to it.toData() }
ApiSuccess(member,activities)

이런 방법은 어떤가요

import kotlinx.serialization.Serializable

@Serializable
data class MemberActivitiesBindActivityTypeApiModel(
Copy link
Collaborator

Choose a reason for hiding this comment

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

제생각엔 이거 저희가 처음 말했던

data class Activity(
id:Long.
type: ActivityType
name:String
)

이 데이터 모델을 공통으로 쓰는게 좋아보이긴 합니다...
만약 이렇게 바꾸는게 이번 스프린트때는 힘들다면 다음 스프린트때 바꾸는게 좋아보여요. 지금 ActivitiesAssociatedByActivityTypeApiModel 이것만 봐도 항상 네이밍 짓기가 되게 힘들 것 같아요.

Comment on lines 9 to 10
val activityType: String,
val memberActivityResponses: List<MemberActivityApiModel>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 개인적으로 부나와 같은 의견으로 명시해주는게 좋아보이긴 합니다.
만약 명시를 안한다고 해도, 저희 3명이 모두 명시를 안한다거나, 모두 명시를 한다거나 하는식으로 통일을 해야할 것 같습니다. (저도 명시를 해주고 있습니다)

companion object {
private val instance = ActivitiesAdapterDecoration()

fun getInstance() = instance
Copy link
Collaborator

Choose a reason for hiding this comment

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

getInstance 함수를 만든 이유가 있을까요...? 저는 instance 의 pirvate 을 빼고 아래 함수를 지우는 것이 좋아보입니다

- fontWeight를 설정해야 텍스트의 굵기마다 맞는 font를 선택할 수 있음
Copy link
Member

@tmdgh1592 tmdgh1592 left a comment

Choose a reason for hiding this comment

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

Approve 하겠습니다 : )

…로필_화면_구현

# Conflicts:
#	android/2023-emmsale/app/build.gradle.kts
@ki960213 ki960213 merged commit 69f1f4a into android-main Aug 1, 2023
0 of 2 checks passed
@ki960213 ki960213 deleted the Feature/#48-프로필_화면_구현 branch August 1, 2023 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android 안드로이드 관련 이슈 High Priority 리뷰 우선순위가 높은 PR 기능 추가 새로운 기능 추가 및 기존 기능 변경
Projects
Status: Archive
Development

Successfully merging this pull request may close these issues.

3 participants