-
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
[Feature] 사장님 운영시간 설정 UI 리뉴얼 #532
Conversation
Update v.4.1.6
Update v4.2.0
Update v4.3.0
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.
고생 많으셨습니다! 리뷰 확인해주세요 :)
business/src/main/java/in/koreatech/business/ui/component/CheckSettingTimeScreen.kt
Outdated
Show resolved
Hide resolved
business/src/main/java/in/koreatech/business/ui/component/CheckSettingTimeScreen.kt
Outdated
Show resolved
Hide resolved
business/src/main/java/in/koreatech/business/ui/component/CheckSettingTimeScreen.kt
Show resolved
Hide resolved
business/src/main/java/in/koreatech/business/ui/component/CheckSettingTimeScreen.kt
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.
수고하셨습니당
고정 dp의 width, height를 쓰는 경우가 종종 있는 거 같은데 다양한 디바이스에 대해서 문제없었을까여?
TimeItem() | ||
} | ||
|
||
val list1: List<String> = listOf( |
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.
정말 좋은 방법인데요!
참고할게요!
"" | ||
) | ||
|
||
val list2: List<String> = listOf( |
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.
이 리스트는 SettingTimeComponent.kt
에서 쓰이는 리스트 같은데 왜 여기에 위치하나여?
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.
원래는 CheckSettingTimeScreen.kt에서 Preview를 위해 사용했던거에요.
그리고 pr에도 남겨놨듯이 로직 적용 전이라서 임시 방편으로 사용한거구요.
로직 구현되면 삭제될 예정이에요!
business/src/main/java/in/koreatech/business/ui/component/CheckSettingTimeScreen.kt
Outdated
Show resolved
Hide resolved
settingTimeList: List<String> = emptyList() | ||
) { | ||
|
||
Divider( |
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.
요거는 VerticalDivider
or HorizontalDivider
와는 다른 컴포넌트인가요?
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.
HorizontalDivider와 동일한 기능을 할거에요!
예전부터 Divider를 써와서 사용했어요!
style = TextStyle( | ||
fontSize = 16.sp, | ||
textAlign = TextAlign.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.
Typography에 정의된 디자인시스템을 따르면 좋을 것 같은데, 새로 TextStyle을 생성해주신 이유가 있을까요?
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.
까먹고 있었어요!
한번 적용해볼게요!
business/src/main/java/in/koreatech/business/ui/component/SettingTimeComponent.kt
Outdated
Show resolved
Hide resolved
Button( | ||
onClick = { | ||
isSettingScreen = false | ||
coroutineScope.launch { | ||
sheetState.hide() | ||
} | ||
}, | ||
colors = ButtonDefaults.buttonColors(Color.White), | ||
shape = RoundedCornerShape(8.dp), | ||
border = BorderStroke(1.dp, Gray3), | ||
modifier = Modifier | ||
.height(44.dp) | ||
.width(128.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.
혹시 버튼은 커스텀한 컴포넌트가 없는건가여?
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.
예전에 만들어둔 버튼이라서 따로 커스텀한 컴포넌트는 없어요
val operatingTime: List<OperatingTimeState> = listOf( | ||
OperatingTimeState("00:00", false, "일", "00:00"), | ||
OperatingTimeState("00:00", false, "월", "00:00"), | ||
OperatingTimeState("00:00", false, "화", "00:00"), | ||
OperatingTimeState("00:00", false, "수", "00:00"), | ||
OperatingTimeState("00:00", true, "목", "00:00"), | ||
OperatingTimeState("00:00", true, "금", "00:00"), | ||
OperatingTimeState("00:00", false, "토", "00:00"), |
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.
수정하겠습니다!
@Composable | ||
fun SettingTime( | ||
modifier: Modifier = Modifier, | ||
storeOperatingTime: List<OperatingTimeState> = 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.
고생하셨습니다!
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.
수고하셨어요!
🐾 개요
🐳 상세 작업
📷 결과 화면
📢 To Reviewers
로직 적용전 변경한 UI만 우선 pr 올립니다!