-
Notifications
You must be signed in to change notification settings - Fork 2
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
[YDS-#228] Component - Toast 구현 #233
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.
수고하셨습니다!
아직 구체적인 구현을 모두 보진 못했고 일단 형식적인 것만 먼저 리뷰 드렸어요. 이거 먼저 수정 부탁드립니다
그리고 왜 이런 방식을 사용해야 하는지, 또는 어떤 코드인지에 대한 설명을 주석으로 달아주시면 좋을 것 같아요 (단순한 구현이 아니다보니 처음 보면 이해하는데 시간이 걸릴 것 같아서요)
compose/src/main/java/com/yourssu/design/system/compose/base/NewScaffold.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/base/NewScaffold.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/foundation/ToastHost.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/foundation/ToastHost.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/foundation/ToastHost.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/foundation/ToastHost.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/component/Toast.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/component/Toast.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/base/YdsScaffold.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/base/YdsScaffold.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/component/Toast.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/component/Toast.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/foundation/ToastAnimation.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/foundation/ToastAnimation.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.
지금 보니 ToastAnimation과 ToastHost가 foundation에 들어가 있는 이유가 무엇인가요?
특별한 이유가 없다면 component 패키지 내부로 옮겨주시면 감사하겠습니다. toast 패키지를 내부에 추가로 만들어도 좋구요
compose/src/main/java/com/yourssu/design/system/compose/component/Toast.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/foundation/ToastAnimation.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/foundation/ToastAnimation.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/foundation/ToastAnimation.kt
Outdated
Show resolved
Hide resolved
아! 그리고 버전도 하나 올려주셔야 라이브러리 배포할 때 반영되니까 버전 올리는 것도 잊지 말아주세요! |
혼자 만드니까 멍청해지네요;; 리뷰가 반드시 필요하겠네요..... |
FadeInFadeOut에 대해서 주석을 작성했는데 이게 좀 복잡해서 제가 주석을 잘 작성했는지가 의심이 되네요.. Comet님 보시고 알려주세요!!! |
넵 감사합니다! 내일 시간 될 때 찬찬히 볼게요 |
그리고 리뷰 반영 커밋은 개수가 많아지더라도 하나씩 나눠서 해주시는 게 확인하기 편할 것 같아요! |
"text for Snackbar expected to have exactly only one child" | ||
} | ||
val textPlaceable = measurables.first().measure(constraints) | ||
val firstBaseline = textPlaceable[FirstBaseline] | ||
val lastBaseline = textPlaceable[LastBaseline] | ||
require(firstBaseline != AlignmentLine.Unspecified) { "No baselines for text" } | ||
require(lastBaseline != AlignmentLine.Unspecified) { "No baselines for text" } | ||
val containerHeight = textPlaceable.height | ||
|
||
Log.d("Toast", "firstBaseline: ${firstBaseline.dp}, lastBaseline: ${lastBaseline.dp}") | ||
|
||
val minHeight = | ||
if (firstBaseline == lastBaseline) { | ||
ToastOneLineMinHeight | ||
} else { | ||
ToastTwoLineMinHeight | ||
} | ||
val containerHeight = max(minHeight.roundToPx(), textPlaceable.height) | ||
layout(constraints.maxWidth, containerHeight) { | ||
val textPlaceY = (containerHeight - textPlaceable.height) / 2 | ||
textPlaceable.placeRelative(0, textPlaceY) |
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.
더이상 Layout으로 위치를 지정해줄 필요 없이 그냥 가운데 정렬만 하면 될 것 같은데 확인 부탁드립니다
(근데 토스트 명세에 텍스트가 한 줄이면 가운데 정렬, 여러 줄이면 왼쪽 정렬해야 하는데 이 부분을 처리하는 로직은 어디에 있는건가요?)
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.align(
// Alignment.Center
// )
를 하기만 해도 텍스트가 한줄일때는 중앙정렬이 되고, 2줄이상 넘어갈때 좌측정렬이 자동으로 됩니다.
원래 방법으로는,
// onTextLayout = {
// lineCount = it.lineCount
// },
를 YdsText에 하고,
// if (lineCount == 1)
// Alignment.Center
// else
// Alignment.CenterStart
하면 정해진 로직대로 구현을 하는 방법입니다. 하지만 첫번째 방법이 이해할 수 없지만 작동하니
이건 어떻게 해야할지 잘 모르겠습니다. (이해도 안되고요...)
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.
아하 그러게요 희한하네요
텍스트 정렬하는 방법을 찾아보니 보통 modifier가 아니라 textAlign 속성을 지정해주는 방식으로 하더라구요 (https://kotlinworld.com/219)
Modifier.align이랑은 무슨 차이가 있을까요..?
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.
내부 구현을 둘러보니까 일단 align 함수는 Box나 Column, Row 스코프 내에서만 사용할 수 있는 함수예요. 컨테이너 내부의 각 요소에 대해 어떻게 둘거냐를 지정하는거죠. 그래서 텍스트의 속성이라고 보긴 어려울 것 같아요.
그리고 Modifier.align(Center)를 하든 안 하든 텍스트 정렬은 TextAlign.Start
(좌측 정렬)가 기본이에요
// 참고
// androidx.compose.ui.text.ParagraphTextStyle
internal val textAlignOrDefault: TextAlign = textAlign ?: TextAlign.Start
근데 왜 말씀하신 것처럼 한 줄일 때는 중앙 정렬이고 여러 줄일 때는 좌측 정렬인 것처럼 보일까요? 그림으로 직관적으로 보여드리자면
이렇게 그냥 텍스트가 짧으면 차지하는 영역이 작으니까 Box의 Center 속성 때문에 가운데에 있는 것처럼 보이는 거예요.
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.
위 링크의 textAlign 속성은 머티리얼 Text에만 있는거더라구요. 그래서 의도대로 하기 위해서는 YdsText를 수정해야 하는데 글로 쓰기는 길어지니까 이 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.
내부 구현을 둘러보니까 일단 align 함수는 Box나 Column, Row 스코프 내에서만 사용할 수 있는 함수예요. 컨테이너 내부의 각 요소에 대해 어떻게 둘거냐를 지정하는거죠. 그래서 텍스트의 속성이라고 보긴 어려울 것 같아요.
그리고 Modifier.align(Center)를 하든 안 하든 텍스트 정렬은
TextAlign.Start
(좌측 정렬)가 기본이에요// 참고 // androidx.compose.ui.text.ParagraphTextStyle internal val textAlignOrDefault: TextAlign = textAlign ?: TextAlign.Start근데 왜 말씀하신 것처럼 한 줄일 때는 중앙 정렬이고 여러 줄일 때는 좌측 정렬인 것처럼 보일까요? 그림으로 직관적으로 보여드리자면
이렇게 그냥 텍스트가 짧으면 차지하는 영역이 작으니까 Box의 Center 속성 때문에 가운데에 있는 것처럼 보이는 거예요.
정말 그렇네요!! 저 이미지가 맞는거 같습니다.
compose/src/main/java/com/yourssu/design/system/compose/base/YdsScaffold.kt
Outdated
Show resolved
Hide resolved
compose/src/main/java/com/yourssu/design/system/compose/component/toast/ToastHost.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.
컴포즈 초짜라 리뷰달건 없지만, 덕분에 많이 배우고 있습니다!
친절한 주석도 감사합니다 :)
scheduledToastData = newToastData // 앞으로 나타날 토스트에 새로운 토스트로 업데이트 | ||
val toastDataList = toastTransitions.map { | ||
it.toastData | ||
}.toMutableList() // 현재 나타난 토스트의 정보로 초기회 |
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.
아주 사소한거지만 주석에 오타가 있네요!
초기회 -> 초기화
저도 잘 모릅니다. 부담가지시지 마시고 정말 무엇이든 물어봐주세요! 사소한 것이라도요. |
@@ -51,6 +51,9 @@ fun YdsScaffold( | |||
} | |||
} | |||
|
|||
@Composable | |||
fun rememberToastHostState(): ToastHostState = remember { ToastHostState() } |
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에서도 해당 함수 사용하는 것으로 수정해주시면 좋을 것 같아요
그리고 이 함수 선언은 ToastHost 파일에 있는 게 더 자연스러울 것 같네요!
* MyScaffold, Toast 초기 후구현 * 구현 완료 * 리뷰 반영 - 기존 YdsScaffold 대체 - preview 맨 아래로, private 변경 - 기존 다른 파일들도 preview function은 전부 private하게 변경 - 인터페이스, enum 맨 위로 - YdsEasing 사용 - Toast Animation 파일 분리 - 기타 코드 정리 * 리뷰 반영 - 코드수정 - 네이밍 변경 - 주석 작성중. - 이해완료 * 리뷰 반영 - 패키지 변경 - 최소높이 제거 - 네이밍 변경 - 주석 작성 * 디버그 용 함수 제거 * rememberToastHostState 생성 * ToastDuration Enum애 시간 정의 * 텍스트 가운데 정렬 로직 추가 * rememberToastHost -> ToastHost.kt로 이동 * 오타 수정
* feat: YdsInAndOutEasing -> YdsEasing & public * [YDS-#228] Component - Toast 구현 (#233) * MyScaffold, Toast 초기 후구현 * 구현 완료 * 리뷰 반영 - 기존 YdsScaffold 대체 - preview 맨 아래로, private 변경 - 기존 다른 파일들도 preview function은 전부 private하게 변경 - 인터페이스, enum 맨 위로 - YdsEasing 사용 - Toast Animation 파일 분리 - 기타 코드 정리 * 리뷰 반영 - 코드수정 - 네이밍 변경 - 주석 작성중. - 이해완료 * 리뷰 반영 - 패키지 변경 - 최소높이 제거 - 네이밍 변경 - 주석 작성 * 디버그 용 함수 제거 * rememberToastHostState 생성 * ToastDuration Enum애 시간 정의 * 텍스트 가운데 정렬 로직 추가 * rememberToastHost -> ToastHost.kt로 이동 * 오타 수정 * refactor: ToastAnimation의 YdsInAndOutEasing -> YdsEasing * Resolve conflict --------- Co-authored-by: Gael-Android <[email protected]>
Summary
Describe your changes
Toast in NewScaffold
Toast
Issue