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

대표 시간표 지정 #209

Merged
merged 12 commits into from
Sep 6, 2023
Merged

Conversation

eastshine2741
Copy link
Collaborator

@eastshine2741 eastshine2741 commented Aug 14, 2023

친구 기능에서 사용할 대표 시간표 지정 기능

(원래 아이콘 수정 PR이었다)

@eastshine2741 eastshine2741 requested a review from a team as a code owner August 14, 2023 10:21
@JuTaK97
Copy link
Collaborator

JuTaK97 commented Aug 16, 2023

이건 아직 진행중인거지?

@eastshine2741
Copy link
Collaborator Author

엥 다 됐어! 아이콘만 수정하는 PR이어서
안그래도 물어볼라그랬는데 대표시간표 아직 시작 안했으면 여기서 내가 쭉 해버릴까?

@JuTaK97
Copy link
Collaborator

JuTaK97 commented Aug 16, 2023

그럼 나는 RN만 집중할게 ㄱㅅㄱㅅ

@eastshine2741 eastshine2741 changed the title TableMoreActionBottomSheet 수정 대표 시간표 지정 Aug 16, 2023
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.

LGTM

@JuTaK97
Copy link
Collaborator

JuTaK97 commented Aug 17, 2023

API 연결 후 머지 ㄱㄱ

changeSelectedTable(siblingTables.first().last().id)
} else {
changeSelectedTable(siblingTables.first()[indexInSibling].id)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

현재 시간표 삭제 로직 수정
현재 학기의 유일한 시간표를 삭제하려는 경우, 삭제하고 전체에서의 인덱스에 따라 다른 시간표를 선택한다

text = stringResource(R.string.home_drawer_table_unset_primary)
) {
scope.launch {
// TODO: API 호출
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

대표시간표 해제 API 나올 때까지 기다리는중

@eastshine2741 eastshine2741 marked this pull request as draft August 20, 2023 14:06
var expanded by remember { mutableStateOf(false) }
LaunchedEffect(table) {
if (courseBook.year == table.year && courseBook.semester == table.semester) {
expanded = true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

현재 시간표를 삭제하여 새로운 table이 선택되면 expanded 여부가 다시 계산되고 있었고,
따라서 현재 시간표를 삭제하여 새로운 table이 선택되었을 때, 열려 있던 다른 coursebook들이 갑자기 닫히는 문제가 있었음
LaunchedEffect로 해결했는데 괜찮아??

Copy link
Collaborator

Choose a reason for hiding this comment

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

그냥 remember에 key를 빼버리면 안 되나?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그러면 현재 테이블을 삭제하여 다른 테이블을 선택하게 되었을 때, 해당 coursebook이 자동으로 expand 되지 않음... 유저 입장에서는 선택된 학기가 갑자기 사라진 것처럼 보이더라구

Copy link
Collaborator

Choose a reason for hiding this comment

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

아항 그러면 어쩔 수 없겠네
LaunchedEffect 저기다 넣어 놔도 잘 동작하면 그렇게 고고

@@ -36,7 +36,7 @@ fun CourseBookDto.toFormattedString(context: Context): String {
4L -> context.getString(R.string.course_book_winter)
else -> "-"
}
return "${this.year} $semesterStr"
return "${this.year} $semesterStr"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NNNN년 N학기

@eastshine2741 eastshine2741 marked this pull request as ready for review September 3, 2023 08:08
@JuTaK97 JuTaK97 force-pushed the eastshine2741/table-more-action-revise branch from 9d57a27 to b0fd4cd Compare September 6, 2023 08:14
@JuTaK97 JuTaK97 enabled auto-merge (squash) September 6, 2023 08:14
@JuTaK97 JuTaK97 merged commit 456974d into develop Sep 6, 2023
3 checks passed
@JuTaK97 JuTaK97 deleted the eastshine2741/table-more-action-revise branch September 6, 2023 08:25
@JuTaK97 JuTaK97 mentioned this pull request Sep 18, 2023
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