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

[Feature] 시간표 마이그레이션 #291

Draft
wants to merge 29 commits into
base: develop
Choose a base branch
from

Conversation

Jokwanhee
Copy link
Contributor

이슈

개요

Java to Jetpack compose migration

시연영상

  • 비로그인
_.2.mp4
_.1.mp4
  • 로그인
_.mp4

추가 논의사항

마이그레이션 하면서 코드 의존성은 강해지고, 코드를 분할해서 PR 날리기 어려워서 코드량이 많은 점 죄송합니다..최대한 분할하여 올렸습니다!!

  • 위젯(Glance)강의 색상 템플릿 & 커스텀 시간표 UI & 다크 모드 대응에 대한 코드는 나중에 PR 올리겠습니다. 참조 레포
  • 비로그인(DataStore) & 로그인(API) 분기처리하여 대응했습니다.
  • 갤러리에 이미지를 비트맵으로 변환하여 저장해야했기 때문에 AndroidView로 커스텀 뷰를 감싸서 사용했습니다.
  • 학과(학부)는 Asset json 파일을 사용하여 진행했습니다.

원하는 MVP는 아니지만, 우선 리뷰를 위하여 PR 올려놓습니다!

질문

  • 현재 바텀시트가 올라오면서 가려진 크기만큼 bottom padding을 주고 있습니다. IME가 올라오면서 바텀시트의 높이가 바뀌면서 여백이 커졌다 줄어지니 흰 배경이 나타났다가 돌아오는 것이 있는데, 해결방법이 있을까요..?

@Jokwanhee Jokwanhee self-assigned this Jun 16, 2024
@Jokwanhee Jokwanhee requested a review from a team as a code owner June 16, 2024 04:19
@Jokwanhee
Copy link
Contributor Author

현재 바텀시트가 올라오면서 가려진 크기만큼 bottom padding을 주고 있습니다. IME가 올라오면서 바텀시트의 높이가 바뀌면서 여백이 커졌다 줄어지니 흰 배경이 나타났다가 돌아오는 것이 있는데, 해결방법이 있을까요..?

해당 문제 해결했습니다.

Copy link
Contributor

@wateralsie wateralsie 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 +19 to +49
fun LectureResponse.toLecture() = Lecture(
id = this.id ?: 0,
code = this.code ?: "",
name = this.name ?: "",
grades = this.grades ?: "",
lectureClass = this.lectureClass ?: "",
regularNumber = this.regularNumber ?: "",
department = this.department ?: "",
target = this.target ?: "",
professor = this.professor ?: "",
isEnglish = this.isEnglish ?: "",
designScore = this.designScore ?: "",
isElearning = this.isElearning ?: "",
classTime = this.classTime,
)

fun TimetablesLectureResponse.toLecture() = Lecture(
id = this.id ?: 0,
code = this.code ?: "",
name = this.name ?: "",
grades = this.grades ?: "",
lectureClass = this.lectureClass ?: "",
regularNumber = this.regularNumber ?: "",
department = this.department ?: "",
target = this.target ?: "",
professor = this.professor ?: "",
isEnglish = "",
designScore = this.designScore ?: "",
isElearning = "",
classTime = this.classTime.orEmpty(),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

lecture에 대해서 두개의 response를 둔 이유가 궁금합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이건 서버에서 불러오는 시간표 api response 스키마가 동일하지 않아서 response dto 를 두개 만들어 놨습니다.. 서버에게 말해야하지만 이번에 v2로 api를 만들고 있는걸로 알아서 공수가 또 들어갈것같아 우선 냅뒀습니다.. 추가로 이전 버전의 사이드 이펙트나 안드로이드가 아닌 다른 플랫폼에서 발생하는 이슈를 생각해보니 하암..

/lectures (강의 목록 조회)
/timetables (시간표 정보 조회)

Column(
modifier = modifier
.fillMaxSize()
.padding(end = 2.dp, bottom = 2.dp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.padding(end = 2.dp, bottom = 2.dp)

시연 영상보니까 각 과목 아이템에 여백이 살짝 있던데 이걸 없애면 시간표 격자에 딱 맞을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

예리한 피드백 감사합니다.
코드량이 많아서 리뷰를 봐주다니 영광이네요..

@kongwoojin
Copy link

현재 바텀시트가 올라오면서 가려진 크기만큼 bottom padding을 주고 있습니다. IME가 올라오면서 바텀시트의 높이가 바뀌면서 여백이 커졌다 줄어지니 흰 배경이 나타났다가 돌아오는 것이 있는데, 해결방법이 있을까요..?

해당 문제 해결했습니다.

고생 많으십니다!
다름이 아니고 해당 부분 코드를 살펴보고 있었는데, 지금 방식 말고 modifier에 imePadding()을 추가하는 방식은 어떨까 싶습니다!

@Jokwanhee
Copy link
Contributor Author

다름이 아니고 해당 부분 코드를 살펴보고 있었는데, 지금 방식 말고 modifier에 imePadding()을 추가하는 방식은 어떨까 싶습니다!

@kongwoojin 피드백 감사드립니다 ^^
말씀해주신 imePadding을 초반 구현에서 진행해봤는데, imePadding 사용 시 WindowCompat.setDecorFitsSystemWindows(window, false)을 사용하면서 인셋을 해제하고 뷰를 확장하게됩니다. 현재 ComposeVIew 로 xml 에 바인딩하고 있고, 인셋문제로 TopBar 가 위로 겹쳐진다거나 이슈가 많아서 동적으로 아래의 패딩넣는 방법을 채택해서 진행했어요~ (더 좋은 방법이 있으시다면 공유해주신다면 감사히 ㅎㅎ..)

@Jokwanhee Jokwanhee marked this pull request as draft July 1, 2024 00:17
onAddLecture: () -> Unit,
onClick: (List<TimetableEvent>) -> Unit,
) {
val isSelected = selectedLecture == lecture
Copy link
Collaborator

Choose a reason for hiding this comment

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

derivedStateOf를 이용하면 lectureselectedLecture가 달라졌지만 isSelected가 유지되는 상황에서 isSelected와 관련있는 컴포저블의 리컴포지션을 방지할 수 있습니다

val isSelected by remember(lecture, selectedLecture) {
  derivedStateOf { selectedLecture == lecture }
}

.padding(start = dayStartPadding)
) {
val days = listOf("월", "화", "수", "목", "금")
repeat(days.size) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(팁) listOf(...).map {} 으로 축약할 수 있을 것 같습니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants