-
Notifications
You must be signed in to change notification settings - Fork 0
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
[ISSUE-45] 약속 잡기 STEP 1-2 구현 #72
Conversation
- POSIX 명세에 따라 파일의 끝에 개행 추가
…-create-plan # Conflicts: # app/src/main/java/com/yapp/growth/App.kt # presentation/src/main/java/com/yapp/growth/presentation/ui/main/PlanzScreen.kt # presentation/src/main/res/values/strings.xml
- 플랜즈 앱 바의 아이콘 content description 추가
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.
코드 잘 봤습니당..!
확실히 가독성이 제 코드보다 훨씬 조은 것 같아요..!
잘 흡수하고 갑니닷 👍👍👍👍👍👍
Timber.plant(object : Timber.DebugTree() { | ||
override fun log(priority: Int, tag: String?, message: String, t: Throwable?) { | ||
super.log(priority, "Debug[$tag]", message, t) | ||
} | ||
}) |
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.
팀버가 로그 찍을 때 위치에 따라서 태그를 알아서 달아주는데, 요 태그에 Debug 문구를 앞에 추가해 주게 됩니당
그러면 태그가 다르게 붙은 여러 곳에서 로그를 찍을 때 내가 찍은 로그만 모아서 검색할 수 있게 돼서 로그 확인할 때 편해용!
viewModel.setEvent(ThemeContract.ThemeEvent.ChoosePlanTheme(it)) | ||
} | ||
) | ||
Spacer(Modifier.height(12.dp)) |
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.
버튼별 간격 조정이면 Column
에서 spacedBy
써보는건 어떨까요오...?
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.
공식문서
근데 spacedBy 는 설정한 패딩으로 아이템들이 모두 간격을 갖게 되는데
지희님이 의도하신건 ThemeChoiceButton 위 44.dp, 아래 12.dp 를 주신거로 봐선
Box 로 감싸서 modifier 에 패딩주는? 방식으로 하든가 해야할 것 같아요
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.
아 맞아요 요고는 바로 column에 탑 패딩 넣고, spacedBy 적용하면 좋을 것 같습니당!
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.
코드 깔끔하게 잘 작성하신거 같아요!
지금 우리 구조대로 sideEffect, event, state 관리하니까
viewModel 내 비즈니스 로직이 깔끔해서 너무 보기 좋네요
Spacer 대신에 속성 이용하는 것만 수정해보면 좋을것 같아요
고생하셨어요!!
👍 👍
@@ -59,23 +59,21 @@ fun PlanzButtonWithBack( | |||
onBackClick: () -> Unit, | |||
) { | |||
Row(modifier = modifier.padding(horizontal = 16.dp)) { |
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.
Row(modifier = modifier.padding(horizontal = 16.dp)) { | |
Row( | |
modifier = modifier.padding(horizontal = 16.dp), | |
horizontalArrangement = Arrangement.spacedBy(10.dp) | |
) { |
아래 Spacer 대신에 Row 의 horizontalArrangement 속성을 사용하는건 어때요?
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.
좋습니다! spacedBy 존재를 몰라서 이런 식으로 작성된 코드들 다시 살펴봐야겠어요!
) { | ||
val textState = getTextState(text = text, maxLength = maxLength) | ||
|
||
Column( |
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.
Column( | |
Column( | |
modifier = modifier.padding(horizontal = 20.dp), | |
verticalArrangement = Arrangement.spacedBy(8.dp) |
로 수정할 수 있겠네요!
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.
좋아요 감사합니당!
|
||
import com.yapp.growth.presentation.R | ||
|
||
enum class PlanThemeType(val themeStringResId: Int) { |
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.
enum 으로 관리하는거 정말 좋은거 같아요 👍
ISSUE
작업 내용
PlanzButton
을 공개하고,PlanzBasicButton
으로 네이밍 변경-> 빈 문자열로 파라미터를 전달할 경우 오류가 발생하기 때문에 빈문자열일 경우 '@'로
BLANK_VALUE
를 추가하여 전달하도록 구현-> 마지막 단계에서
BLANK_VALUE
일 경우 빈문자열로 치환하여 서버로 전송해야 함!추가)
실행 화면
Check List
[ISSUE-{N}] PR 제목
으로 작성