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

[FEAT] 코스 발견 / 상세페이지 타 유저 프로필 조회 #305

Merged
merged 30 commits into from
Jan 10, 2024

Conversation

sxunea
Copy link
Collaborator

@sxunea sxunea commented Dec 29, 2023

📌 개요

✨ 작업 내용

  • 코스 발견 -> 코스 상세페이지에서 작성 유저 클릭 시 해당 유저 프로필 조회하는 화면 구현했습니다
    • 유저 프로필 조회 API 연결 + 기능 구현
    • 유저 업로드한 코스 스크랩 API 연결 + 기능 구현

✨ PR 포인트

  • 유저의 스탬프 이미지가 서버에서 받아오는 latestStamp로 지정되는데, 기존 MyPageFragment의 구현을 보면 BindingAdapter+getStampResId 라는 Extension의 조합으로 되어있더라구요, 이를 합쳐서 새로운 BindingAdapter(3ab1d5e) 로 만들어 주었습니다 ! 이에 대한 의견과 혹시 이전에 저렇게 따로 두었던 이유가 있었는지 궁금합니다 !

  • binding 하는 시점마다 클릭리스너를 등록하고 있는데, 이부분은 저번 회의 때 결론 대로 일단 두었습니다 ~! 업데이트 배포 후 다른 것들과 함께 논의해보면 좋을 것 같아요

  • 플로우 자체가 코스 발견 -> 코스 상세페이지 -> 에서 유저 클릭 -> 유저 프로필 -> 에서 업로드한 코스 클릭 -> 다시 코스 상세페이지 인데, 재진입한 상세페이지에서 뒤로 갈 시, 어느정도의 지체 + 깜빡임이 생기면서 (구현 영상 참고) 프로필로 돌아가게됩니다. 눈에 띄는 오류는 없어서 PR 먼저 올리고 해결해 보는중입니다😢

📸 스크린샷/동영상

Screen_Recording_20231230-022422_Runnect.mp4

@sxunea sxunea added FEAT ✨ 새로운 기능 구현 혜선 🐱 혜선 담당 labels Dec 29, 2023
@sxunea sxunea self-assigned this Dec 29, 2023
Copy link
Member

@dongx0915 dongx0915 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~!
연말이라 diff로 간단하게만 봤는데 시간 날 때 좀 더 꼼꼼하게 볼게요!

@@ -600,6 +620,7 @@ class CourseDetailActivity :
private const val EXTRA_COURSE_DATA = "CourseData"
private const val EXTRA_FRAGMENT_REPLACEMENT_DIRECTION = "fragmentReplacementDirection"
private const val EXTRA_FROM_COURSE_DETAIL = "fromCourseDetail"
private const val EXTRA_COURSE_USER_ID = "userId"
Copy link
Member

Choose a reason for hiding this comment

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

해당 상수의 값을 상수 명과 동일하게 가져가는 건 어떨까요?
userId라는 값은 다른 값과 겹칠 가능성이 있어 보입니다.

액티비티 간 데이터 전달을 위한 키 값이라 다른 곳에서 겹칠 일은 없겠지만 이런 키 값들은 최대한 안 겹치게 작성해주는 게 사이드 이펙트가 적을 것 같아요

회사에서 작업할 때 서로 다른 곳에서 동일한 키 값을 사용하게 되어 이슈가 생긴 적이 있었습니다.

당장은 겹칠 일이 없어보여도 명확하게 써서 나쁠 건 없다고 봅니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

감사합니다 ! 안그래도 publicCourseId, courseId, userId 다양해서 고민이었는데 동일하게 가져가는 편이 훨씬 편할 것 같습니다 반영해둘게요 👍

Comment on lines 39 to 40
viewModel.postCourseScrap(courseId = courseId, scrapTF = scrapTF)
adapter.updateCourseItem(courseId = courseId, scrapTF = scrapTF)
Copy link
Member

Choose a reason for hiding this comment

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

postCourseScrap의 결과와는 상관 없이 View에 업데이트를 해주고 있는건가요??
요청에 실패했을 경우엔 좋아요가 유지되고 있을까요??

Copy link
Member

@leeeha leeeha Jan 5, 2024

Choose a reason for hiding this comment

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

post 서버 통신에 성공했을 때만 뷰를 갱신시키는 방향으로 수정이 필요해보입니다! 그래서 서버 응답값으로 스크랩한 코스의 id가 필요하다고 회의 때 얘기했던 거군요!

@dongx0915
Copy link
Member

dongx0915 commented Dec 31, 2023

코스 발견 탭 -> 코스 선택 -> 유저 프로필 클릭 -> 코스 선택 -> 유저 프로필 클릭

이 플로우에서 뒤로가기 2번 누르면 Exception 발생하고 있습니다!
영상에서 깜빡거리는 부분이 크래시 때문인 것 같아요

2023-12-31 16:01:51.576 30593-30593/com.runnect.runnect E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.runnect.runnect, PID: 30593
    kotlin.UninitializedPropertyAccessException: lateinit property rootScreen has not been initialized
        at com.runnect.runnect.presentation.detail.CourseDetailActivity.navigateToPreviousScreen(CourseDetailActivity.kt:159)
        at com.runnect.runnect.presentation.detail.CourseDetailActivity.handleBackButtonByCurrentScreenMode(CourseDetailActivity.kt:147)
        at com.runnect.runnect.presentation.detail.CourseDetailActivity.access$handleBackButtonByCurrentScreenMode(CourseDetailActivity.kt:63)
        at com.runnect.runnect.presentation.detail.CourseDetailActivity$registerBackPressedCallback$callback$1.handleOnBackPressed(CourseDetailActivity.kt:122)
        at androidx.activity.OnBackPressedDispatcher.onBackPressed(OnBackPressedDispatcher.java:255)
        at androidx.activity.OnBackPressedDispatcher$$ExternalSyntheticLambda1.run(Unknown Source:2)
        at androidx.activity.OnBackPressedDispatcher$Api33Impl$$ExternalSyntheticLambda0.onBackInvoked(Unknown Source:2)
        at android.view.ViewRootImpl$NativePreImeInputStage.onProcess(ViewRootImpl.java:7558)
        at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:7213)
        at android.view.ViewRootImpl.deliverInputEvent(ViewRootImpl.java:10788)
        at android.view.ViewRootImpl.doProcessInputEvents(ViewRootImpl.java:10676)
        at android.view.ViewRootImpl.enqueueInputEvent(ViewRootImpl.java:10632)
        at android.view.ViewRootImpl$WindowInputEventReceiver.onInputEvent(ViewRootImpl.java:10926)
        at android.view.InputEventReceiver.dispatchInputEvent(InputEventReceiver.java:285)
        at android.os.MessageQueue.nativePollOnce(Native Method)
        at android.os.MessageQueue.next(MessageQueue.java:335)
        at android.os.Looper.loopOnce(Looper.java:186)
        at android.os.Looper.loop(Looper.java:313)
        at android.app.ActivityThread.main(ActivityThread.java:8762)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:604)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1067)

@dongx0915
Copy link
Member

현재 플로우 상 '코스 선택 -> 유저 프로필 -> 코스 선택 -> 유저 프로필 -> ...' 처럼 화면이 무한으로 쌓일 수 있는 구조인데, Intent Flag 값을 통한 관리가 필요해보입니다!

Copy link
Contributor

@unam98 unam98 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!!

@@ -12,6 +12,22 @@ fun ImageView.setLocalImageByResourceId(resId: Int) {
load(resId)
}

@BindingAdapter("setStampImageByResourceId")
fun ImageView.setStampImageByResourceId(stampId: String?) {
Copy link
Contributor

Choose a reason for hiding this comment

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

만들어둔 하나의 함수가 여러 곳에서 쓰일 수 있으면 재사용성 측면에서 좋을 텐데 만약 getStampResId 확장함수를 제거하고 이 logic을 BindingAdapter에 합쳐버린다면 추후에 이미지뷰에 바인딩하는 목적 말고도 단순히 리소스ID값만 조회하고 싶은 상황이 있을 땐 다시 logic을 작성해주어야 하니까 재사용성이 떨어지겠다는 생각이 들었습니다.

그런데 한편으로는 이미 getStampResId라는 함수가
서버에서 값을 보내주면 이걸 활용해 local에 저장돼있는 이미지와 대조해 부합하는 리소스ID를 반환 으로
쓰임 목적이 뚜렷한 특수한 함수이기 때문에 애초에 재사용성이 떨어지는 함수일 수밖에 없다는 생각도 들었습니다.

결론은, 애초에 재사용성에 한계가 있는 상황임을 고려한다면 혜선님이 제안해주신 방향을 채택하는 게 절차를 간소화시킬 수 있다는 점에서 좋다고 생각합니다!! 👍

제가 짠 logic이 아니라 잘은 모르지만 아마 따로 두었던 것에 특별한 이유는 없었을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 저도 리소스 경로가 지정되었다는 점에서는 확장성이 좀 떨어지겠다는 생각이 있었는데 같은 고민을 하셨군요 😢
일단은 저 함수가 마이페이지와 유저페이지 모두에서 서버에서 보내준 값에 따라 프로필 이미지를 binding 하는데만 쓰이고 있으니까 의견처럼 지금대로 가고, 추후 다른 용도로 필요하다면 더 고려해보도록 할게요 !!

Comment on lines 61 to 77
fun toUserProfile(): UserProfileData {
val userCourseDataList: List<UserCourseData> = courses.map { course ->
UserCourseData(
publicCourseId = course.publicCourseId,
courseId = course.courseId,
title = course.title,
image = course.image,
departure = DepartureData(
region = course.departure.region,
city = course.departure.city,
town = course.departure.town,
detail = course.departure.detail,
name = course.departure.name ?: ""
),
scrapTF = course.scrapTF
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

mapper 쪽에서부터 null 시 대체값 지정을 통해 서버에 문제가 생긴 상황에서도 앱이 안 죽게 핸들링 해보면 좋을 것 같아요! 저도 그동안 코드에서는 반영을 못했고 최근에서야 느낀 생각인데 위클리 때 한 번 논의해보면 좋을 것 같습니다 :)

@@ -129,6 +134,14 @@ class CourseDetailActivity :
applyScreenExitAnimation()
}

private fun navigateToUserProfileWithBundle() {
Copy link
Contributor

Choose a reason for hiding this comment

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

함수 네이밍 중 WithBundle은 어떤 의미인가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

EXTRA_COURSE_USER_ID 를 Bundle 형태로 보내줘서 저렇게 지어뒀습니다 !

Copy link
Contributor

Choose a reason for hiding this comment

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

data를 Intent로 넘기고 있지 않나요?!

Intent랑 Bundle이랑 둘 다 data를 넘기는데 활용될 수 있지만 서로 다른 개념이라 여쭤봅니다!

Copy link
Member

Choose a reason for hiding this comment

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

    public @NonNull Intent putExtra(String name, @Nullable String value) {
        if (mExtras == null) {
            mExtras = new Bundle();
        }
        mExtras.putString(name, value);
        return this;
    }

Intent 내부에서도 Bundle을 쓰고 있긴한데, navigateToUserProfileWithBundle 메소드에서 직접 Bundle을 생성하여 전달하는 것은 아니니 WithBundle이 필요해보이진 않습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 Intent가 내부로는 Bundle을 쓰는거라 그렇게 사용하곤 했는데 맞는 말씀이네요 수정하도록 할게요 ~! 감사합니다

Comment on lines +58 to +62
private fun initBackButtonClickListener() {
binding.ivProfileBack.setOnClickListener {
finish()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

추후 다른 listener들이 추가 될 때 모아볼 수 있게 setUpListener()라는 포괄적인 함수 안에 listener들을 모아 넣는 게 어떨까요?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

요 화면에서는 리스너가 하나여서 나중에 만들까했는데 확장성을 고려하면 미리 해두는게 좋겠네요 ! 감사합니다 :)

return@launch
}
_userProfileState.value = UiStateV2.Success(profileData)
Timber.e("GET PROFILE DATA SUCCESS")
Copy link
Contributor

Choose a reason for hiding this comment

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

Timber.e 를 쓰시는 이유가 있나요? e는 에러 메세지 띄울 때 쓰는 것으로 알고 있어서요!

Copy link
Collaborator Author

@sxunea sxunea Jan 3, 2024

Choose a reason for hiding this comment

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

엇 용도는 그렇긴한데 빨간색이라 애용했..습니다 잘보여서😅 Success에 활용되니 그래도 e는 쓰지 말아야겠네요 ! 꼼꼼한 리뷰감사합니다

Copy link
Member

@dongx0915 dongx0915 Jan 3, 2024

Choose a reason for hiding this comment

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

안드로이드 스튜디오 설정에서 로그 타입 별로 색상을 정해줄 수 있으니 error를 쓰는 것보단 이걸 활용해보면 좋을 것 같아요~!

Copy link
Member

Choose a reason for hiding this comment

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

오오 꿀팁 감사합니다!! 저도 Timber.e 많이 썼었는데 색상 바꾸는 방법이 있다는 건 처음 알았네요!

val publicCourseId: Int,
val courseId: Int,
val title: String,
val image: String,
val departure: DepartureData,
val scrapTF: Boolean
var scrapTF: Boolean,
)

Copy link
Member

Choose a reason for hiding this comment

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

서버통신 응답값에서 가공을 거친 데이터 클래스들은 보통 domain 레이어에 두는 경우가 많은데, data 레이어에 위치시킨 이유가 있나요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗 저거 mockData 작업할 때 쓰던거 변경한거라 위치 이동을 빼먹었네요 😲 감사합니다 : )

val levelPercent: Int,
val latestStamp: String,
val courseData: List<UserCourseData>
)
Copy link
Member

@leeeha leeeha Jan 5, 2024

Choose a reason for hiding this comment

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

네이밍에 관한 개인 취향 차이일 수 있지만, UserProfileData, UserCourseData 모두 그 자체로 데이터 클래스이므로 굳이 끝에 Data 라는 이름을 붙이지 않아도 될 거 같습니다 :)

Comment on lines 39 to 40
viewModel.postCourseScrap(courseId = courseId, scrapTF = scrapTF)
adapter.updateCourseItem(courseId = courseId, scrapTF = scrapTF)
Copy link
Member

@leeeha leeeha Jan 5, 2024

Choose a reason for hiding this comment

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

post 서버 통신에 성공했을 때만 뷰를 갱신시키는 방향으로 수정이 필요해보입니다! 그래서 서버 응답값으로 스크랩한 코스의 id가 필요하다고 회의 때 얘기했던 거군요!

putExtra(EXTRA_COURSE_USER_ID, courseDetail.userId)
startActivity(this)
}
applyScreenExitAnimation()
Copy link
Member

Choose a reason for hiding this comment

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

유저 프로필 화면으로 들어갈 때는 exit 대신에 enter 애니메이션 사용하는 게 자연스러울 거 같아요! :)

Copy link
Member

Choose a reason for hiding this comment

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

@sxunea 이 부분도 반영 부탁드립니다!


class ProfileCourseAdapter(
) : ListAdapter<ProfileCourseData, ProfileCourseAdapter.UploadedCourseViewHolder>(diffUtil) {
private val onLikeButtonClick: (Int, Boolean) -> Unit,
Copy link
Member

Choose a reason for hiding this comment

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

버튼 모양은 하트지만, 러넥트에서는 이게 스크랩 기능이다 보니 onLikeButtonClick 대신 다른 네이밍을 사용하면 더 좋을 거 같아요 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

onScrapButtonClick 으로 네이밍 변경하도록 할게요 ~! 꼼꼼한 리뷰 감사합니다

sxunea added 5 commits January 6, 2024 15:44
데이터 클래스의 이름엔 Data 제거
스크랩 버튼 클릭시 해당 코스의 정보를 뷰모델에 Pair로 저장

데이터 클래스의 이름엔 Data 제거
@sxunea sxunea requested review from dongx0915, leeeha and unam98 January 6, 2024 07:49
putExtra(EXTRA_COURSE_USER_ID, courseDetail.userId)
startActivity(this)
}
applyScreenExitAnimation()
Copy link
Member

Choose a reason for hiding this comment

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

@sxunea 이 부분도 반영 부탁드립니다!

getCourseDetail()
}
}

Copy link
Member

Choose a reason for hiding this comment

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

유저 프로필 화면에서 코스 상세페이지로 전환할 때, CourseDetailActivity가 이미 백스택에 있기 때문에 onNewIntent 함수에서 다시 상세페이지를 조회한 것으로 이해하면 될까요??

나중에 이 코드만 보면 이게 뭐지?! 할 수도 있을 거 같아서 주석을 간단하게 적어두는 것도 괜찮을 거 같습니다!

scrapCourseData.let { data ->
adapter.updateCourseItem(courseId = data.first, scrapTF = data.second)
}
}
Copy link
Member

@leeeha leeeha Jan 7, 2024

Choose a reason for hiding this comment

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

서버한테 응답값 수정해달라고 요청하는 것이 좋을 거 같습니다 🙏 클라단에서 코드가 훨씬 간결해질 거 같아서요!
수린이가 담당인 것으로 알고 있어요!

Copy link
Member

@leeeha leeeha left a comment

Choose a reason for hiding this comment

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

변경사항 확인 완료했습니다!

@sxunea sxunea merged commit f676d4c into develop Jan 10, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FEAT ✨ 새로운 기능 구현 혜선 🐱 혜선 담당
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 코스 발견 / 상세페이지 타 유저 프로필 조회
4 participants