-
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
refactor: preview 수정 및 state Hoisting 적용 #197
base: refactor-state-hoisting
Are you sure you want to change the base?
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.
고생하셨습니다. 😁
...iz/src/main/java/kr/boostcamp_2024/course/quiz/presentation/question/QuestionDetailScreen.kt
Outdated
Show resolved
Hide resolved
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/quiz/src/main/java/kr/boostcamp_2024/course/quiz/component/QuizSolveTimeSlider.kt
Outdated
Show resolved
Hide resolved
...iz/src/main/java/kr/boostcamp_2024/course/quiz/presentation/question/CreateQuestionScreen.kt
Outdated
Show resolved
Hide resolved
@@ -136,22 +138,39 @@ fun QuestionDetailScreen( | |||
} | |||
} | |||
|
|||
@Preview | |||
class QuestionDetailScreenPreviewParameterProvider : PreviewParameterProvider<Question> { |
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.
다양한 오픈소스를 참고해 보니 ParameterProvider
역시 internal
키워드를 붙여 모듈 내부에서만 활용하더라고요! 그리고 만약 여러 모듈에서 함께 활용되는 Provider라면 app 모듈에 두기도 하더라고요! 이 부분도 오늘 이야기 나눠봅시다! 🙂
...uiz/src/main/java/kr/boostcamp_2024/course/quiz/presentation/quiz/GeneralQuizResultScreen.kt
Outdated
Show resolved
Hide resolved
val quizResultPreviewQuestions = | ||
listOf( | ||
ChoiceQuestion( | ||
id = "1", | ||
title = "1번 문제", | ||
solution = null, | ||
description = "1번 문제 설명", | ||
answer = 1, | ||
choices = listOf(), | ||
userAnswers = listOf(), | ||
), | ||
ChoiceQuestion( | ||
id = "2", | ||
title = "2번 문제", | ||
solution = null, | ||
description = "2번 문제 설명", | ||
answer = 1, | ||
choices = listOf(), | ||
userAnswers = listOf(), | ||
), | ||
BlankQuestion( | ||
id = "3", | ||
title = "3번 문제 (낱말 맞추기)", | ||
solution = null, | ||
userAnswers = emptyList(), | ||
questionContent = 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.
이 부분은 사용되지 않는 것 같은데 맞을까요~?
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.
앗 사용되고 있었군요!! 제가 잘못 봤나 보네요😅
혹시 PreviewParameterProvider
를 적용하지 않고 변수로 선언해 여러 프리뷰에서 활용하는 이유가 있을까요?
변수로 선언하여 활용하는 것보다 provider를 활용하는 것이 여러 곳에서 해당 데이터가 사용된다는 것을 명시적으로 나타낼 수 있는 것 같으네 훈님 생각은 어떠신가요~?
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.
PreviewParameterProvider
는 여러 프리뷰를 그려줄 때 사용하는 용도라고 생각하고 그냥 일반 변수로 지정해뒀는데,
조금 찾아보니 지금처럼 여러 프리뷰에서 쓰이는 경우에도 사용해도 좋을 거 같다고 생각이 들었습니다~!
provider로 빼보겠습니다! 좋은 의견 감사합니다~!
…factor-state-hoisting-K041
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.
회의에서 정한 preview 관련 convention이 잘 지정 되어 있네요! 수고하셨습니다!🙂
@Preview(showBackground = true, locale = "ko") | ||
@PreviewLightDark | ||
@Preview(uiMode = UI_MODE_NIGHT_YES or UI_MODE_TYPE_NORMAL, locale = "ko") | ||
@Composable |
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.
해당 부분은 @PreviewKoLightDark가 적용되어 있지 않네요! 다른 부분도 @PreviewKoLightDark
가 적용되어 있지 않은 부분이 꽤 있는 것 같아요! 확인 부탁드립니다!🙂
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.
@PreviewKoLightDark
가 적용 안된 컴포넌트들은 showBackground = true가 없으면 프리뷰가 이상하게 나와서 따로 설정해주었습니다😢
따로 어노테이션을 구성해야할지 (PreivewKoLightDarkWithBackground
..???ㅋㅋㅋ) 아니면 이대로 둘지 토욜날 정해보죠..!
++ 영민님이 만드신 어노테이션이랑 맞춰서 구성하겠습니다!!
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.
접근 제한자를 붙이지 않은 이유가 따로 있으실까요?
fun NotificationScreen(
notificationInfos: List<NotificationWithGroupInfo>,
onRejectClick: (String) -> Unit,
onAcceptClick: (Notification) -> Unit,
onNavigationButtonClick: () -> Unit,
snackBarHostState: SnackbarHostState,
)
fun QuestionResultItem(
questionResult: QuestionResult,
onQuestionClick: (String) -> Unit,
)
fun CreateStudyScreen(
isEditMode: Boolean,
defaultStudyImageUri: String?,
currentStudyImage: ByteArray?,
... 생략
onStudyEditButtonClick: () -> Unit,
onCreationButtonClick: () -> Unit,
onCurrentStudyImageChanged: (ByteArray) -> 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.
아니 스크린에 제한자를 다 빼먹었군요... 요번주에 혼이 나가있어서 실수가 많네요 수정하겠습니다!
…nParameterProvider클래스로 분리 (#196)
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.
수고 많으셨습니다! 머지할 때 겹치는 부분은 제가 수정해서 올리도록 할게요 😄
|
||
@Preview(locale = "ko", showBackground = true) | ||
@Preview(locale = "ko", showBackground = true, uiMode = UI_MODE_NIGHT_YES) | ||
annotation class PreviewKoLightDarkBackground |
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.
굿입니다😄
👩🌾 진행한 작업
✅ NotificationScreenPreview 언어 한글로 변경
✅ CreateQuestionScreenPreview 오류 해결, 언어 한글로 변경
✅ CreateStudyScreenPreview 언어 한글로 변경
✅ OwnerQuizResultScreenPreview 오류 수정, 언어 한글로 변경
✅ GeneralQuizResultScreenPreview 오류 수정, 언어 한글로 변경
✅ QuestionDetailScreenPreview 오류 수정, 언어 한글로 변경
✅QuizOwnerDialogPreview 네이밍 수정, dark prievew 추가, 언어 ko로 설정
✅QuizSolveTimeSliderPreview dark prievew 추가, 언어 ko로 설정
✅QuizTopAppBarPreview 추가
✅RadioTextButtonPreview 추가
✅RealTimeQuestionPreview 추가
✅RealTimeQuizGuideContentPreview 추가
✅quiz component internal keyword 적용
✅각 스크린 및 프리뷰 다크모드 적용
QuestionDetailScreenPreview
🗣️ 공유할 내용
프리뷰 살린 방법
프리뷰는 태환님의 블로그 글을 통해 오류를 해결하였습니다.
간단하게 설명드리면 저희가 상태를 가지고 있게 한 최상단 컴포저블이 Preview는 동작하지 않지만 외부에서 활용할 코드로 활용하는 용도였더라고요!
PreviewParameterProvider 활용
QuestionDetailScreenPreview는 객관식과 낱말맞추기 두개의 프리뷰가 필요합니다.
이 두개를 PreviewParameterProvider를 사용하면 프리뷰 한개로 각각 랜더링이 가능합니다~
드로이드 나이츠 참고하다 발견했고, 이 블로그 글을 읽으면 이해가 되실겁니다!
미리보기용 데이터 String 추출 여부
드로이드 나이츠를 보니 미리보기용 데이터에서 사용되는 String을 추출한것도있고 아닌곳도 있더라고요!
무슨 근거로 각각 그렇게 구성했는지 확인 후 저희 모두 일괄적으로 적용하면 좋을 것 같습니다!
State Hoisting
분명 브랜치 이름은 State Hoisting인데 원래도 다들 신경써주셔서 나름 잘 되어있어서 건들게 없었습니다😅
하지만 제가 공부하면서 느끼지만, 불필요한 것도 hoisting이 되어있다고 생각합니다!예를들어CreateQuestionScreen
에서 snackBarHostState와 focusRequester가 호이스팅 되어있는데,Stateless하게 구성한 컴포저블 함수의 안에서만 쓰이기 때문에 호이스팅을 하여 얻는 이점이 거의 없는거 같습니다.
오히려 프리뷰에서도 snackBarHostState랑 focusRequester를 선언해줘야해서 불편함이 커집니다..!
공식문서에서도 무조건 호이스팅이 옳은게 아니다 라고 나와있고, (내부에서만 사용되면 호이스팅의 이점이 거의 없다 서술)저희 프로젝트에서도 다른 스크린에선 snackBarHostState는 호이스팅을 안한 곳도 있더라고요.
~~ 제 기준에서 생각했을땐 snackBarHostState정도는 state를 내려줘도 될거같은데, 여러분의 생각도 듣고싶습니다~
(제 코드에는 토론 후에 적용하려고 snackBarHostState에 대한 호이스팅 변경은 하지 않았습니다~)
(토요일날 토론해봐도 좋을 거 같아요~)
--> 확장성 때문에 최상단의 stateful한 컴포저블에서 snackBarHostState를 관리하는게 좋다고 생각이 바뀌었습니다!
학습의 끈이 짧았습니다. . . . 😥
알림 (01/15 18:00 작성)
showBackground 남아있는 컴포넌트는 없으면 프리뷰가 잘 안보여서 넣어줬습니다.
RadioTextButton은 다크모드의 차이가 없어서 안 넣어 주었습니다.
closed #196