-
Notifications
You must be signed in to change notification settings - Fork 0
feat: 서재 UI/UX 개편(2) - 서재뷰 구현 #720
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
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 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
s9hn
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.
긴 작업 고생하셨습니다! 모듈화가 진행된 만큼 전체적으로 가시성 제어자를 점검해주시면 좋을 것 같아욧! ex) internal
| LaunchedEffect(Unit) { | ||
| libraryViewModel.novelList.collect { | ||
| Log.d("123123", it.toString()) | ||
| val uiState by libraryViewModel.uiState.collectAsState() |
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: collectAsStateWithLifecycle로 변경하면 좋을 것 같아요
| import kotlinx.coroutines.flow.map | ||
|
|
||
| @Composable | ||
| fun LibraryScreen(libraryViewModel: LibraryViewModel = hiltViewModel()) { |
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.
c: 현재는 LibraryScreen의 테스트가 힘든 구조예요! stateful/stateless한 스크린함수로 분리해도 좋을 것 같아요
| fun onSortClick(selected: SortTypeUiModel) { | ||
| val newSortType = when (selected.sortType) { | ||
| SortType.NEWEST -> SortType.OLDEST | ||
| SortType.OLDEST -> SortType.NEWEST | ||
| } | ||
| mutableQueryParams.update { it.copy(sortType = newSortType) } | ||
| _uiState.update { it.copy(selectedSortType = SortTypeUiModel.from(newSortType)) } | ||
| } | ||
|
|
||
| fun onFilterClick(type: LibraryFilterType) { | ||
| // TODO: 바텀시트 연결 예정 | ||
| } | ||
|
|
||
| fun onItemClick(item: LibraryListItemModel) { | ||
| // TODO: 상세화면 이동 | ||
| } | ||
|
|
||
| fun navigateToExplore() { | ||
| // TODO: 탐색화면 이동 | ||
| } |
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.
c: 다소 UI 의존적인 네이밍같아요. 함수의 책임이 명확히 드러나도록 해주세요
| Column( | ||
| modifier = Modifier.fillMaxSize(), | ||
| @Composable | ||
| fun LibraryTopBar(onSearchClick: () -> Unit = {}) { |
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.
a: 탑바만 같은 파일에 위치하는 이유가 있나요?
- onSeachClick에 기본생성자를 둔 이유도 궁금해요.
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.
탑바만 같은 파일에 위치한 이유는.. 사실 크게 없는데요 ㅎㅎ 그냥 확장성이나 재사용성이 크지 않은 부분이라고 생가해서 같은 파일에 두었던 것 같습니다! 그리고 onSearchClick을 기본 생성자로 둔 이유는 아직 구현되지 않아도 되는 부분이라서 onSearchClick은 필수로 넘기지 않아도 된다고 생각했습니다! 그래서 기본값을 설정해놓고 해당 부분에 대한 관리포인트를 줄이고 싶어서 기본 생성자로 두었습니다!
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.
나중에 다른 개발자가 서재의 탑바를 수정할 때 찾기 쉽도록 분리하면 좋을 것 같아요!
| private val mutableQueryParams = MutableStateFlow(LibraryQueryParams()) | ||
| private val queryParams: StateFlow<LibraryQueryParams> = mutableQueryParams.asStateFlow() |
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.
c: 외부에 공개되는 상태값이 아니라면, 굳이 백킹프로퍼티를 사용하지 않아도 될 것 같아요.
| .size(width = size.width, height = size.height) | ||
| .clip(RoundedCornerShape(8.dp)), | ||
| ) { | ||
| AsyncImage( |
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: core의 networkImage를 사용해주세요 !
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.
이런게 있는 줄 몰랐네요!!
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 backgroundColor = if (isSelected) Black else White | ||
| val textColor = if (isSelected) White else Gray300 | ||
|
|
||
| Surface( |
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.
c: surface를 사용한 이유가 있나요?
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.
선택 상태에 따라서 배경색과 테두리를 유연하게 바꾸는 구조를 만들기 위해서 surface를 사용했습니다!
| } | ||
|
|
||
| @Composable | ||
| fun SortTypeSelector( |
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.
a: 이곳은 왜 public한가요?
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.
이건 저의 꼼꼼함이 미치지 못한 부분인것같네요.. 수정하겠습니다
| private val INPUT_DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd") | ||
| private val OUTPUT_DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyy.MM.dd") | ||
|
|
||
| fun formatDateRange( |
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.
c: 어느뷰든 수정없이 활용가능한 유틸인가요?
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.
오 넵! 그런 구조로 만들기는했습니다!
formatDateRange(item.startDate, item.endDate)?.let { Text( text = it, style = WebsosoTheme.typography.label2, color = Gray200, ) } 이런식으로 활용하면 됩니다!
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.
그럼 core의 common으로 빼두어도 좋을 것 같아요
| package com.into.websoso.feature.library.util | ||
|
|
||
| fun buildFilterLabel( | ||
| defaultLabel: String, | ||
| values: List<String>, | ||
| ): String = | ||
| when { | ||
| values.isEmpty() -> defaultLabel | ||
| values.size == 1 -> values.first() | ||
| else -> "${values.first()} 외 ${values.size - 1}개" | ||
| } |
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.
a: 어떤 의미의 '유틸'인지, 유틸이란 패키지가 필요한지 의문입니다 !
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.
말씀 주신 부분에 저도 공감하며, 실제로 util 패키지에 대해 고민이 있었습니다! 처음 해당 패키지를 만든 건 날짜 포맷팅 함수처럼 리스트/그리드 아이템에서 공통으로 사용하는 로직을 재사용 가능한 위치에 두기 위함이었습니다. 이번 buildFilterLabel 함수 역시 리스트 아이템 내에서 사용되고 있는데 관련 컴포저블의 코드가 다소 길어져서 UI 로직과 분리해보기 위해 유틸로 뺐던 상황입니다. 다만 말씀처럼 util이라는 이름이 다소 포괄적이고 책임이 모호하게 느껴질 수 있다는 점에 동의합니다. 그래서 현재 구조보다 더 적절한 위치나 네이밍이 있다면 개선해보고 싶습니다. 혹시 추천해주실 방향이 있으실까요? 😊
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.
저라면 로직의 성격에 따라 뷰모델 혹은 UI모델에 위치할 것 같아요! 가급적이면 Compose는 정말 UI 껍데기로 취급하고 정제된 상태값만을 넘기고 싶거든요! 가능한 로직을 다루지 않으려해요 :)
📌𝘐𝘴𝘴𝘶𝘦𝘴
📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯
📷𝘚𝘤𝘳𝘦𝘦𝘯𝘴𝘩𝘰𝘵
💬𝘛𝘰 𝘙𝘦𝘷𝘪𝘦𝘸𝘦𝘳𝘴
서버 통신 관련해서 유즈케이스를 연결했을 때 문제가 발생해서 산군이 한 번 확인을 해주시면 감사하겠습니다!! 그리고 코드가 많이 길어져서 죄송해요... 스크린샷은 해당 이슈 해결되면 올리겠습니다!! 빌드는 이상 없습니다!
[고민포인트]
일단 생각보다 서재 자체에 요소가 많아서 열심히 분리를 해봤는데 너무.. 분리를 한걸까..?라는 고민을 좀 했습니다... 보시고 의견 남겨주시면 감사하겠습니다!@!