-
Notifications
You must be signed in to change notification settings - Fork 1
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/#306] 3차 스프린트 탐색 뷰 #320
Conversation
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.
수고하셨습니다!!
혹시 배너 API 연결하는 코드는 어디서 확인할 수 있나요..?!
그리구 영상에 보이는 1월 4일 공사중이라는 배너는 올라가 있으면 안돼요..!! 이 배너 어디서 가져온건가용?
fun SearchRoute( | ||
modifier: Modifier, | ||
paddingValues: PaddingValues, | ||
viewModel: SearchViewModel = hiltViewModel(), | ||
navigateToSearchProcess: () -> Unit, | ||
navigateToIntern: (Long) -> Unit, | ||
viewModel: SearchViewModel = 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.
옵셔널한 값은 가장 아래로 오게 순서 수정 부탁드립니당
modifier = Modifier | ||
.padding(vertical = 12.dp), | ||
horizontalArrangement = Arrangement.spacedBy(12.dp), | ||
contentPadding = androidx.compose.foundation.layout.PaddingValues(horizontal = 24.dp) |
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.
androidx.compose.foundation.layout
는 임포트로 넣어주는 게 좋을 것 같아요!
modifier = Modifier | |
.padding(vertical = 12.dp), | |
horizontalArrangement = Arrangement.spacedBy(12.dp), | |
contentPadding = androidx.compose.foundation.layout.PaddingValues(horizontal = 24.dp) | |
modifier = Modifier | |
.padding(vertical = 12.dp), | |
horizontalArrangement = Arrangement.spacedBy(12.dp), | |
contentPadding = PaddingValues(horizontal = 24.dp) |
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.
진짜 배너 연결 부분이 없네요!! 한번만 확인 부탁드립니다 :)
@@ -103,7 +104,7 @@ fun SearchRoute( | |||
|
|||
@Composable | |||
fun SearchScreen( | |||
modifier: Modifier = Modifier, | |||
paddingValues: PaddingValues, | |||
bannerList: List<com.terning.domain.search.entity.SearchBanner>, |
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.
수고하셨습니당~ 배너 api는 아직 PR이 안올라온거 같은데 후딱 올려주세여!
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 bannerState by viewModel.bannerState.collectAsStateWithLifecycle(lifecycleOwner = lifecycleOwner) | ||
val viewState by viewModel.viewState.collectAsStateWithLifecycle(lifecycleOwner = lifecycleOwner) | ||
val scrapState by viewModel.scrapState.collectAsStateWithLifecycle(lifecycleOwner = lifecycleOwner) |
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.
궁금해서 물어보는 건데요...! 혹시 bannerState
를 따로 추가해준 이유가 있나용...?! state를 나누는 기준이 궁금해졌어요!
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.
배너 / 조회수 많은 공고 / 스크랩 많은 공고로 state를 나누었습니다!
val bannerList = when (bannerState.searchBannersList) { | ||
is UiState.Success -> (bannerState.searchBannersList as UiState.Success<List<com.terning.domain.search.entity.SearchBanner>>).data.toImmutableList() | ||
else -> emptyList() | ||
} | ||
|
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.
흠 그리고 feature에서 domain의 데이터클래스를 사용하는 거에 대해 괜찮을지에 대한 의문이 드네요..
feature에서 사용할 모델을 따로 만들어 주는 건 어떤가요.....?
val bannerList = when (bannerState.searchBannersList) { | |
is UiState.Success -> (bannerState.searchBannersList as UiState.Success<List<com.terning.domain.search.entity.SearchBanner>>).data.toImmutableList() | |
else -> emptyList() | |
} | |
val bannerList = when (bannerState.searchBannersList) { | |
is UiState.Success -> (bannerState.searchBannersList as UiState.Success<List<SearchBanner>>).data.toImmutableList() | |
else -> emptyList() | |
} |
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.
헉 감사해욤 feature에 있는 모델 State로 변경했습니다
private val _bannerList: MutableStateFlow<List<com.terning.domain.search.entity.SearchBanner>> = | ||
MutableStateFlow(emptyList()) | ||
|
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.
여기도 임포트 수정 부탁드립니다!
_sideEffect.emit(SearchSideEffect.Toast(R.string.server_failure)) | ||
} |
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.
엇 제가 여기는 수정을 못한 것 같네요..! 서버통신 성공여부 문구는 designsystem의 R클래스를 공통으로 사용하면 통일성 있을 것 같습니당,,!
searchBanners: List<SearchBanner>, | ||
onAdvertisementClick: (Int) -> 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.
저희 List는 ImmutableList로 수정해주면 감사하겠습니다🙇♀️
searchBanners: List<SearchBanner>, | |
onAdvertisementClick: (Int) -> Unit, | |
searchBanners: ImmutableList<SearchBanner>, | |
onAdvertisementClick: (Int) -> Unit, |
HorizontalPager( | ||
state = pagerState, | ||
modifier = modifier, | ||
beyondViewportPageCount = 1 | ||
) { currentPage -> | ||
val pageIndex = currentPage % searchBanners.size | ||
AsyncImage( | ||
model = ImageRequest.Builder(LocalContext.current) | ||
.data(searchBanners[pageIndex].imageUrl) | ||
.crossfade(true) | ||
.build(), | ||
contentDescription = null, | ||
modifier = modifier | ||
.fillMaxWidth() | ||
.height(112.dp) | ||
.noRippleClickable { onAdvertisementClick(pageIndex) }, | ||
contentScale = ContentScale.Crop, |
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.
여기 두 부분 모두 Modifier로 바꿔줘야 할 것 같아요,,!!
HorizontalPager( | |
state = pagerState, | |
modifier = modifier, | |
beyondViewportPageCount = 1 | |
) { currentPage -> | |
val pageIndex = currentPage % searchBanners.size | |
AsyncImage( | |
model = ImageRequest.Builder(LocalContext.current) | |
.data(searchBanners[pageIndex].imageUrl) | |
.crossfade(true) | |
.build(), | |
contentDescription = null, | |
modifier = modifier | |
.fillMaxWidth() | |
.height(112.dp) | |
.noRippleClickable { onAdvertisementClick(pageIndex) }, | |
contentScale = ContentScale.Crop, | |
HorizontalPager( | |
state = pagerState, | |
modifier = Modifier, | |
beyondViewportPageCount = 1 | |
) { currentPage -> | |
val pageIndex = currentPage % searchBanners.size | |
AsyncImage( | |
model = ImageRequest.Builder(LocalContext.current) | |
.data(searchBanners[pageIndex].imageUrl) | |
.crossfade(true) | |
.build(), | |
contentDescription = null, | |
modifier = Modifier | |
.fillMaxWidth() | |
.height(112.dp) | |
.noRippleClickable { onAdvertisementClick(pageIndex) }, | |
contentScale = ContentScale.Crop, |
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.
확인했습니다! 아직 페이징 처리가 안되어있어서 이 부분만 추가로 마무리 해주시면 감사하겠습니다!!
override suspend fun getSearchBannersList(): Result<List<SearchBanner>> = | ||
kotlin.runCatching { | ||
searchDataSource.getSearchBanners().result.toSearchBannerList() | ||
} |
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.
이거 이제 봤는데 kotlin 지워줘야 할 것 같아요!
override suspend fun getSearchBannersList(): Result<List<SearchBanner>> = | |
kotlin.runCatching { | |
searchDataSource.getSearchBanners().result.toSearchBannerList() | |
} | |
override suspend fun getSearchBannersList(): Result<List<SearchBanner>> = | |
runCatching { | |
searchDataSource.getSearchBanners().result.toSearchBannerList() | |
} |
viewModel.getSearchList( | ||
keyword = state.text, | ||
page = 0, | ||
size = 0 | ||
size = 100 | ||
) | ||
} |
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.
혹시 여기 100으로 해준 이유가 있나요..?
제가 알기론 이 부분 페이징 처리해줘야 된다고 알고 있는데 아마 10개씩 불러와야 할 거에요!
⛳️ Work Description
📸 Screenshot
306.mp4
📢 To Reviewers