-
Notifications
You must be signed in to change notification settings - Fork 1
feat: 작품 피드 장르별 색상 반영 #800
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
base: release/1.6.0
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| ) | ||
|
|
||
| // 1. 첫번쨰 카테고리를 가져온다 | ||
| val firstCategory = feed.relevantCategories.first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r: null 처리가 번거로우시겠지만, 가급적이면 firstOrNull과 같은 safe api를 사용해주세요!
생각보다 non-null api들에서 익셉션이 빈번히 발생합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 반영하여 수정하였습니다 ! 감사합니다 👍
app/src/main/java/com/into/websoso/ui/main/feed/adapter/FeedViewHolder.kt
Outdated
Show resolved
Hide resolved
기존의 복잡한 분기문으로 처리하던 피드 아이템의 장르 및 색상 결정 로직을 `firstOrNull`을 사용하여 간소화했습니다.
m6z1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'기타' 장르일 경우, 배경에 1dp 회색 테두리를 추가하고 기존 색상 코드는 그대로 유지하도록 수정했습니다. 이를 위해 기존 `setBackgroundTint` 바인딩 어댑터를 `setGenreBackground`로 변경하고, 장르 이름에 따라 분기 처리하는 로직을 추가했습니다.
수정하는 과정에서 필요없는 코드가 작성되어 있다면 알려주시면 감사하겠습니다 ! Screen_recording_20260201_133925.mp4 |
m6z1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨어용
| @ColorRes | ||
| private var novelBackgroundColor: Int? = null | ||
|
|
||
| @ColorRes | ||
| private var novelIconColor: Int? = null | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 생각했던 방향과는 조금 다른 것 같습니다.
ViewModel에서 장르 정보를 받아 UI 전용 모델로 조립해서 전달해 달라고 말씀드린 이유는, Adapter가 색상과 같은 UI 상태를 직접 가지지 않도록 하기 위함이었습니다.
현재 FeedAdapter는 앱 여러 곳에서 재사용되는 공용 피드 어댑터입니다.
이름 그대로 FeedItem과 RecyclerView, ViewHolder를 연결하는 역할만 수행해야 하며, 특정 화면의 디자인 요구사항과 강하게 결합된 상태값을 내부에 가지는 것은 적절하지 않다고 생각합니다.
만약 이후 디자인 요구사항이 확장되어 색상뿐 아니라 특정 아이콘, 레이아웃 조건 등이 추가된다면, 그때마다 Adapter에 UI 상태 필드를 계속 추가해야 하는 구조가 됩니다. 이는 재사용성과 유지보수 측면에서 좋지 않습니다.
Adapter는 아이템을 바인딩하는 객체 역할에만 집중하도록 구현해 주세요.
특정 화면에서 필요한 UI 요구사항은, 해당 화면 전용 UI 모델을 통해 전달하는 방식이 바람직합니다.
즉,
ViewModel → 화면 전용 UI Model 생성
Adapter → UI Model을 그대로 바인딩만 수행
하는 구조로 가져가면 좋겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 감사합니다 !
Adapter가 특정 화면의 UI 상태(색상, 아이콘 등)를 내부에 가지지 않고 ViewModel에서 화면 전용 UI Model을 완성해서 전달하는 구조를 원하신 것으로 이해했습니다.
이에 따라 기존에 Adapter 내부에서 장르 색상 상태를 관리하던 로직을 제거하고
ViewModel에서 장르 정보를 받아 FeedModel(novel 영역)에
backgroundColor / iconColor / genre 값을 포함한 UI 전용 모델을 생성하여 emit하도록 수정했습니다.
현재 FeedAdapter는 공용 어댑터로서
전달받은 FeedModel을 그대로 바인딩만 수행하며,
색상이나 장르에 대한 계산 로직은 가지지 않도록 정리했습니다.
특정 화면(NovelFeed)에서만 필요한 UI 요구사항은
ViewModel 단계에서 처리되도록 구조를 변경한 상태입니다.
추가로 보완이 필요한 부분이 있다면 알려주시면 감사하겠습니다 !
피드 화면에서 각 피드 아이템에 작품의 대표 장르와 그에 맞는 배경색이 표시되도록 기능을 추가했습니다. 작품 상세 정보에서 대표 장르를 가져와 피드 뷰모델에 전달하고, 이를 각 피드 모델에 적용하여 UI에 반영합니다. - '기타' 장르일 경우, 테두리가 보이도록 스타일을 조정했습니다. - 작품 상세 ViewModel에서 대표 장르를 관리하고, 이를 피드 Fragment와 ViewModel에서 관찰하여 사용합니다. - 피드 데이터 로딩 및 갱신 시, 대표 장르 정보가 함께 적용되도록 로직을 수정했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제 의도대로 잘 작성해주셨습니다!
다만 기존 웹소소 컨벤션에 부합하지 못하는 변경사항이 있어 그 부분만 가이드해드렸습니다!
해당 코멘트만 반영 부탁드리고, 궁금한 점 있으시면 질문 남겨주세요!
천천히 적용해주셔도 됩니다 :)
- 리뷰가 조금 난잡할것 같아서 총정리한번 드리면,
- 뷰모델에서 발생한 모든 변경사항은 롤백해도 됩니다!
feedDetailViewModel.novelDetailModel.novel.getGenres.firstOrNull() ?: "기타"값이 곧 우리가 사용할 장르입니다.- 따라서 어댑터에 submitList하려는 기존리스트에 위 코드를 넣어주면(copy and map) 됩니다.
- 장르별 필요한 UI요소(아이콘색상, 배경색상,stroke, drawable) 등은 FeedModel이 생성자로 genre를 받았으므로, 이를 활용해 내부에 로직으로 구현해주시면 됩니다.
- (선택) 바인딩 어댑터 함수의 경우 Context가 필요하다면 함수 형태로 Context를 인자로 받고 이를 활용해 결과값을 Return하는 유틸성 함수를 UiModel 내부에 구현해주셔도 되고, 뷰홀더에서 구현해주셔도 됩니다(가급적 바인딩어댑터보단)
| @ColorRes | ||
| private var novelBackgroundColor: Int? = null | ||
|
|
||
| @ColorRes | ||
| private var novelIconColor: Int? = null | ||
|
|
||
| fun updateGenre(genre: String) { | ||
| val normalizedGenreName = genre.trim().ifEmpty { ETC } | ||
| novelGenreName = normalizedGenreName | ||
|
|
||
| val category = Category.from(normalizedGenreName) | ||
| novelBackgroundColor = category.backgroundColor | ||
| novelIconColor = category.iconColor | ||
|
|
||
| val state = _feedUiState.value ?: return | ||
| _feedUiState.value = state.copy( | ||
| feeds = applyNovelGenre(state.feeds), | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import android.* 코드가 없도록 유지(AAC관련 코드 제외, android.arch.*)
웹소소 뷰모델 컨벤션이 있습니다! 안드로이드 의존성은 제거해주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
거의 다 잘해주셨는데, 기존에 저희 컨벤션에 맞게 위 리뷰만 수정되면 좋을 것 같습니다 :)
지금 작성해주신 updateGenre를 FeedUiModel로 옮기면 쉽게 끝날 것 같아요!
| @ColorRes val iconColor: Int? = null, | ||
| ) { | ||
| val isNothing: Boolean = id == null | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| val genre: String = "", | |
| @ColorRes val backgroundColor: Int? = null, // TODO: 제거 | |
| @ColorRes val iconColor: Int? = null, // TODO: 제거 | |
| ) { | |
| private val normalizedGenreName: String get() = genre.trim().ifEmpty { ETC } | |
| private val category: Category get() = Category.from(normalizedGenreName) | |
| val novelBackgroundColor: Int = category.backgroundColor | |
| val novelconColor: Int = category.iconColor | |
| } |
이렇게 작성하면 genre 하나만 넘겨줌으로써 해당 아이템이 필요한 정보가 내부에 캡슐화될 것 같아요!
| novelDetailViewModel.primaryGenre.observe(viewLifecycleOwner) { genre -> | ||
| novelFeedViewModel.updateGenre(genre) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 프래그먼트(NovelFeedFragment)의 생성시점은 상위 엑티비티 및 뷰모델이 초기화된 이후 시점이므로, 굳이 옵저빙하지 않아도 될 것 같아요!
| private val _primaryGenre = MutableLiveData(ETC) | ||
| val primaryGenre: LiveData<String> get() = _primaryGenre | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래 리뷰에 따라 NovelDetailViewModel도 롤백 가능합니다!
|
|
||
| private fun updateFeeds(novelFeedUiState: NovelFeedUiState) { | ||
| binding.clNovelFeedNone.isVisible = novelFeedUiState.feeds.isEmpty() | ||
| val feeds = novelFeedUiState.feeds.map { Feed(it) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 더 명확하고 좋은 방식은,
NovelDetailViewModel의 UiState('genre'를 가지고 있는)와 NovelFeedViewModel의 UiState(실제 피드데이터)
두 뷰모델의 UiState를 각각 스트림(Flow)으로 만들어 bind 시켜주는 방법입니다(두 스트림을 하나의 최종 스트림으로 합치는 작업).
해당 작업은 스트림에 대한 이해와 적지 않은 코드 변경사항을 요구하므로 현재단계에선 단순하게 아래처럼 불러와도 괜찮을 것 같습니다!
| val feeds = novelFeedUiState.feeds.map { Feed(it.copy(genre = novelDetailViewModel.novelDetailModel.novel.getGenres.firstOrNull() ?: "기타")) } |
| @BindingAdapter(value = ["dynamicBackgroundColor", "genreName"], requireAll = false) | ||
| fun View.setGenreBackground( | ||
| @ColorRes colorRes: Int?, | ||
| genreName: String?, | ||
| ) { | ||
| val drawable = background?.mutate() as? GradientDrawable ?: return | ||
|
|
||
| if (colorRes != null) { | ||
| drawable.setColor(ContextCompat.getColor(context, colorRes)) | ||
| } | ||
|
|
||
| val normalized = genreName?.trim() | ||
| val isEtc = normalized.isNullOrEmpty() || normalized == "기타" | ||
|
|
||
| if (isEtc) { | ||
| val strokeWidth = (1 * context.resources.displayMetrics.density).toInt() | ||
| val strokeColor = ContextCompat.getColor(context, R.color.gray_70_DFDFE3) | ||
| drawable.setStroke(strokeWidth, strokeColor) | ||
| } else { | ||
| drawable.setStroke(0, 0) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
바인딩 어댑터는 일종의 확장함수입니다!
팀마다 바인딩어댑터를 어떤 컨벤션으로 활용하는지에 따라 다를 수 있겠지만, 보통은 XML에서 사용할 하나의 attribute로써 범용적인 함수를 구현합니다.
현재 setGenreBackground는 앞서 얘기했듯 특정 도메인과 강하게 결합된 함수로 바인딩어댑터 함수 성격과는 조금 맞지 않는다고 생각해요!
가급적 UI로직은 UI모델로 캡슐화해주세요! 지금 이 함수의 구현체도 Ui모델 내부에서 잘 풀 수 있을 것 같습니다
구현에 어려움이 있다면 그대로 두셔도 괜찮습니다!
| @@ -177,6 +179,7 @@ | |||
| android:layout_marginTop="16dp" | |||
| android:layout_marginBottom="16dp" | |||
| android:src="@drawable/ic_link" | |||
| app:imageTint="@{feed.novel.iconColor}" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제 UiModel 예시대로라면, feed.novel.novelBackgroundColor, feed.novel.novelIconColor 로 수정하면 될 것 같습니다!

📌𝘐𝘴𝘴𝘶𝘦𝘴
📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯
📷𝘚𝘤𝘳𝘦𝘦𝘯𝘴𝘩𝘰𝘵
Screen_recording_20260128_001553.mp4
Screen_recording_20260128_002659.mp4
로그도 함께 첨부합니다 !

💬𝘛𝘰 𝘙𝘦𝘷𝘪𝘦𝘸𝘦𝘳𝘴
현재 relevantCategories에서 값을 가져오고 있는데, 한 장르로 고정된 값이 아니라 장르가 계속 바뀌어서 색상이 다 다르게 나오는 중입니다..
하지만 보시는 바와 같이 두번째 영상은 의도한대로 같은 색상으로 나오고 있습니다 🥲
이해하시기 편하도록 주석을 달아뒀는데 추후에 제거하겠습니다 !