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

관심강좌, 알림 아이콘 이동 #228

Merged
merged 17 commits into from
Jan 3, 2024

Conversation

eastshine2741
Copy link
Collaborator

  • 관심강좌
    • TimetablePage -> SearchPage로 이동
    • Topbar UI 리팩토링
    • AnimatedContent로 탭 전환 효과
  • 알림
    • SettingsPage -> TimetablePage로 이동

@eastshine2741 eastshine2741 requested a review from a team as a code owner November 29, 2023 13:23
Copy link
Collaborator

@JuTaK97 JuTaK97 left a comment

Choose a reason for hiding this comment

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

넘예뿌다

Comment on lines 21 to 32
sealed class SearchPageMode(val page: Int) {
data object Search : SearchPageMode(0)
data object Bookmark : SearchPageMode(1)

fun toggled(): SearchPageMode {
return when (this) {
is Search -> Bookmark
is Bookmark -> Search
}
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 파일 분리하자

Comment on lines 86 to 89
LaunchedEffect(Unit) {
searchViewModel.pageMode.collect {
pagerState.animateScrollToPage(
page = it.page,
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 코드랑 pagerState 의 존재 이유는 뭐지?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 원래 앱바는 AnimatedContent고 리스트는 HorizontalPager여서 pagerState를 썼는데 리스트도 AnimatedContent로 바꿔서 필요없는 코드가 됨... 지우는 걸 까먹었네
리스트를 HorizontalPager에서 AnimatedContent로 바꾼 이유는 제목 바뀌는 거랑 싱크가 안맞아서.. indicator나 탭이 있는 것도 아니고 해서 바꿨어

Comment on lines 101 to 86
is SearchPageMode.Search -> {
slideInHorizontally { width -> -width } + fadeIn() togetherWith
slideOutHorizontally { width -> width } + fadeOut() using SizeTransform(clip = false)
}
is SearchPageMode.Bookmark -> {
slideInHorizontally { width -> width } + fadeIn() togetherWith
slideOutHorizontally { width -> -width } + fadeOut() using SizeTransform(clip = false)
}
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

Choose a reason for hiding this comment

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

팩트) 고수맞다

Comment on lines 220 to 217
SearchPageMode.Search -> SearchResultList(
scope,
searchResultPagingItems,
searchViewModel,
timetableViewModel,
tableListViewModel,
lectureDetailViewModel,
userViewModel,
vacancyViewModel,
reviewBottomSheetWebViewContainer,
)
SearchPageMode.Bookmark -> BookmarkList(
searchViewModel,
timetableViewModel,
tableListViewModel,
lectureDetailViewModel,
userViewModel,
vacancyViewModel,
reviewBottomSheetWebViewContainer,
)
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.

언젠가...... 커스텀테마 끝나고 해볼까...

Comment on lines 428 to 455
@Composable
fun BookmarkPlaceHolder() {
Column(
modifier = Modifier
.fillMaxSize(),
verticalArrangement = Arrangement.spacedBy(5.dp),
horizontalAlignment = Alignment.CenterHorizontally,
) {
Spacer(modifier = Modifier.weight(1f))
Text(
text = stringResource(R.string.bookmark_page_placeholder_1),
style = SNUTTTypography.subtitle1.copy(
fontSize = 18.sp,
color = SNUTTColors.White700,
fontWeight = FontWeight.Bold,
),
)
Text(
text = stringResource(R.string.bookmark_page_placeholder_2),
style = SNUTTTypography.subtitle1.copy(fontSize = 18.sp, color = SNUTTColors.White700),
)
Text(
text = stringResource(R.string.bookmark_page_placeholder_3),
style = SNUTTTypography.subtitle1.copy(fontSize = 18.sp, color = SNUTTColors.White700),
)
Spacer(modifier = Modifier.weight(1f))
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

파일 분리하자 SearchPage.kt 너무 길어

Comment on lines 67 to 72
try {
fetchSearchTagList() // FIXME: 학기가 바뀔 때마다 불러주는 것으로 되어 있는데, 여기서 apiOnError 붙이기?
fetchSearchTagList()
getBookmarkList()
} catch (e: Exception) {
apiOnError(e)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixme 놓친거 있길래 고쳤어요
VacancyViewModel에서 한 것처럼 ApiOnError 주입받아서 했슴다

@eastshine2741 eastshine2741 force-pushed the eastshine2741/move-bookmark-page branch from a3cb235 to a1b8e2d Compare January 1, 2024 17:58
@eastshine2741 eastshine2741 merged commit bdca815 into develop Jan 3, 2024
3 checks passed
@eastshine2741 eastshine2741 deleted the eastshine2741/move-bookmark-page branch January 3, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants