Conversation
feature: breakfast Dining items
feature: can scrollable
feature: dining compose
fix: fix to strings.xml
feature: dining bottom sheet
fix: feat ktlint
…o refactor/dining-compose # Conflicts: # core/navigation/src/main/java/in/koreatech/koin/core/navigation/NavigatorType.kt # koin/src/main/java/in/koreatech/koin/navigation/NavigatorImpl.kt # koin/src/main/java/in/koreatech/koin/ui/main/activity/MainActivity.kt
|
현재 Navigation 이 바뀌면서 기존 NotificationActivity 로 이동하는 방식이 달라져야합니다. |
kongwoojin
left a comment
There was a problem hiding this comment.
고생하셨습니다.
추가로 비로그인 상태에서 식단 진입 시 /notification API를 호출해 인증 정보 만료가 뜨는데, 비로그인 상태 분기처리가 필요할 것 같습니다.
추가로 nestedScroll 적용과 conflict 수정도 해주세요
| @@ -0,0 +1,116 @@ | |||
| package `in`.koreatech.koin.core.designsystem.component.switch | |||
There was a problem hiding this comment.
designsystem으로 빼는게 원래는 맞겠지만... 디자인 팀에서 제공한 디자인이 아니기도 하고 디자인 시스템 리팩토링이 진행중이라 일단은 feature/dining 모듈에 넣는게 어떨까 싶습니다.
| type: Pair<String, Any?> = Pair("", "") | ||
| ): Intent | ||
|
|
||
| fun navigateToNotification( |
There was a problem hiding this comment.
navigateToNotificationSetting이 좀 더 맞을 것 같습니다.
혹은 navigateToSetting으로 만들고 enum 하나 만들어서 세부 화면을 관리해도 될 것 같네요
| implementation(libs.coil.gif) | ||
|
|
||
| implementation(libs.timber) | ||
| implementation(project(":core:onboarding")) |
| modifier: Modifier = Modifier, | ||
| onClick: (Date) -> Unit = {} | ||
| ) { | ||
| val currentDate = TimeUtil.getCurrentTime() |
| @Composable | ||
| fun DiningItem( | ||
| dining: Dining, | ||
| context: Context, |
There was a problem hiding this comment.
context를 넘기지 않고 LocalContext.current를 사용해도 되지 않나요
There was a problem hiding this comment.
뭔가 개별 compose 에서 LocalContext.current 를 가져오는 것 보다 하나의 LocalContext.current 를 가져오는 변수를 screen 에서 만들고 뿌려주는 방식을 생각했습니다. context 가 바뀌어도 쉽게 대응이 가능하니까요.
다만 context 가 바뀔 일이 없고 귀찮게 context를 자꾸 파라미터로 받아야 하는 상황이 되었습니다.
그냥 개별적으로 LocalContext.current 쓰겠습니다
| @Composable | ||
| fun KoinSwitch( | ||
| checked: Boolean, | ||
| onCheckedChange: () -> Unit, |
There was a problem hiding this comment.
onCheckedChange에서 변경 이후의 Boolean을 넘겨주는 것이 맞을 것 같습니다
| navController: NavController | ||
| ) { | ||
| composable( | ||
| route = "${DiningNavType.DiningDetail.route}?initDate={${INIT_DATE}}&initTabType={${INIT_TAB_TYPE}}", |
There was a problem hiding this comment.
route 형태를 이렇게 만든 이유가 있나요
혹시 레퍼런스가 있다면 알려주세요
There was a problem hiding this comment.
기존 DiningActivity 로 가는 경로는(위젯, 설정에서의 식단) 아무런 데이터 없이 그냥 DiningActivity 로 이동합니다. 다만 카카오 공유를 통해 들어오면 특정 요일, 특정 시점(아,점,저)의 정보를 넘겨주고 있어서 그에따라 이동해야합니다.
initDate 와 initTabType 를 기본 파라미터로 주고 initDate 를 activity 나 navigation 에서 계산하는 방법이 있는데 그건 좀 아닌거같아 선택적 파라미터로 주고 그 값이 없으면 viewModel 내부에서 계산하도록 했습니다.
There was a problem hiding this comment.
아뇨 ${DiningNavType.DiningDetail.route}/{$INIT_DATE}/{$INIT_TAB_TYPE} 형태로 해도 되는데 굳이 ${DiningNavType.DiningDetail.route}?initDate={${INIT_DATE}}&initTabType={${INIT_TAB_TYPE}}로 작성하신 이유가 있나 해서 여쭤본겁니다
There was a problem hiding this comment.
확실히 ${DiningNavType.DiningDetail.route}/{$INIT_DATE}/{$INIT_TAB_TYPE} 로 하고 {$INIT_DATE}/{$INIT_TAB_TYPE} 값을 기본값으로 넘겨주면 되긴 하겠네요.
저는 그런 부분은 생각 안했고 선택적 파라미터를 통해 route 를 작성하면 ${DiningNavType.DiningDetail.route} 만 route 값을 줘도 자동으로 {$INIT_DATE}/{$INIT_TAB_TYPE} 값을 default 값으로 주고 navigation 이 동작하기에 저렇게 만들었습니다. 그냥 / 로 하면 어쨋든 필수값을 끼워서 route 로 써야하니까요.
인터넷 url에서도 필수값이 아닌 값은 /search?q=ABC 처럼 사용하듯 만들었습니다.
| showTooltip: Boolean, | ||
| showBottomSheet: Boolean, | ||
| experimentGroup: String, | ||
| context: Context, |
There was a problem hiding this comment.
마찬가지로 context를 넘길 필요는 없을 것 같습니다
|
|
||
| @OptIn(ExperimentalFoundationApi::class, ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| fun DiningDetailScreenImpl( |
| @@ -0,0 +1,16 @@ | |||
| package com.example.dining | |||
| Text( | ||
| text = date.toInstant().atZone(ZoneId.systemDefault()).toLocalDate().dayOfMonth.toString(), | ||
| style = KoinTheme.typography.medium15, | ||
| color = if (isSelected) { |
| Column( | ||
| modifier = modifier | ||
| .fillMaxWidth() | ||
| .shadow( |
There was a problem hiding this comment.
shadow가 2개 있는데 이거 맞는건지 확인 부탁드립니다.
There was a problem hiding this comment.
맞는거더라구요. 저도 좀 놀라긴 했습니다만 디자인 쪽에서 shadow는 대부분 2가지를 동시에 입혀서 표현하는 거 같습니다. 명암차이로 더 입체적으로 보이기 위해서 아닐까요?
| style = KoinTheme.typography.bold18 | ||
| ) | ||
| Spacer(Modifier.height(12.dp)) | ||
| Column( |
There was a problem hiding this comment.
이미 수직 방향 column안에 수직 방향 column을 사용하고 있습니다. 이거 없이도 Spacer만 사용하여 구현할 수 잇을 것 같습니다.
There was a problem hiding this comment.
spacer 만으로 간단하게 구현이 가능하긴 합니다.
다만 개인적으로 비슷한 내용을 포함하는 ui 요소들은 하나의 box 나 column, row 등으로 묶는 편입니다. 이부분도 코드 컨벤션을 통해 맞춰나가는게 좋을 거 같습니다.
There was a problem hiding this comment.
depth가 깊어진다는 문제가 있지만, padding과 arrangement 때문에 묶는 것 자체는 괜찮을 것 같긴 합니다.
다만 비슷한 내용을 포함하는 ui 요소들을 묶는 것이 목적이라면 차라리 별도의 Composable 함수로 분리하는 것이 어떨까 싶네요 (component 디렉토리에 분리 X, 같은 파일에 있는 함수)
| ) | ||
| } | ||
| } | ||
| Column( |
There was a problem hiding this comment.
이부분도 마찬가지로 dialog 에 확인, 취소 버튼처럼 버튼 구조의 두가지를 코드적으로 묶기 위해 처리했습니다.
fix: fix to navigation
fix: context default
fix: switch onValueChange
fix: fix to private screenimpl
fix: nested if to when
fix: feat ktlint
fix: fix getNotiPermissionInfo work only userState is not anonymous
fix: delete unused type
…o refactor/dining-compose
fix: feature unused import
fix: change to feature DiningActivity
refactor: change scroll system to use nestedScroll
fix: change shaow value more realistic
fix: redesgin
fix: feat ktlint
fix: delete unused value
fix: add loading screen
|
기존 피드백 모두 수정했습니다. 기존 dining 화면으로 변경된 부분을 다시 feature dining ui 로 바꾸었습니다. |
이미지 클릭 시 다이얼로그에서 확대 가능해야 합니다. |
현재 다이얼로그에서 더블클릭으로 인한 확대는 안되어도 |
기존에는 더블 탭으로 확대가 가능하지 않았나요 기능은 동일해야 할 것 같습니다 |
Upgrade imageDialog gestures system and animation scroll system
Delete coerceIn boilerplate
Feat ktlint
|
최대한 기존 image Dialog 와 비슷하게 만들었습니다. 더블탭 확대, 스크롤 확대 모두 지원합니다. |
kongwoojin
left a comment
There was a problem hiding this comment.
아니다 Toolbar height가 여전히 이상하네요
|
아하 길다고하신게 collapsing tool bar 가 아니라 top app bar 였군요 |
Dining detail screen consume windowsInsets
Delete consume windowsInsets
Top app bar height is limited 85dp
DiningViewModel extends ViewModel() instead BaseViewModel
Add RefreshToPull Event
Feat Ktlint
TopAppBar 높이 감소원본 component 가 높이가 고정이 되어있어서... heightIn 을 사용해 85.dp 로 제한했습니다. (수작업으로 찾기) BaseViewModel() 상속 을 ViewModel() 상속으로 변경BaseViewModel 내부 기능도 잘 사용하지 않고 필요도 없어서 제거했습니다. (내부의 LiveData 가 보기 싫었어요) PullToRefresh 적용기존 Dining 처럼 pull to refresh 적용했습니다. |
Add dining caution text and bottom padding
NavigationBarPadding 추가windowinsets 를 모두 0 으로 만드니 edge to edge 로 하단 navigationbar 와 겹치는 상황 발생 식단 변경 알림문구 추가기존 dining 의 요소인 식단 변경 알림 문구 추가
|
Add weekend photo text and image dialog logging
누락된 Image Dialog 클릭 logging 추가주말 사진은 "주말은 식단 이미지를 제공하지 않습니다." 안내 문구가 나오도록 수정 |
| containerColor = KoinTheme.colors.neutral0, | ||
| topBar = { | ||
| KoinTopAppBar( | ||
| modifier = Modifier.heightIn(max = 85.dp), |
There was a problem hiding this comment.
아 늘리신게 아니라 기본값이 애초에 다른거네요
이러면 기본값으로 두는게 맞나... 아...
|
|
||
| init { | ||
| fun initData() { | ||
| getDining(initDate) |
There was a problem hiding this comment.
dining은 init으로 처리하고, user 관련된 부분만 snapshotflow로 처리하면 어떨까요?
There was a problem hiding this comment.
MutableStateFlow 가 많아서 그런지
이제 init 안에 MutatbleStateFlow값이 있으면 null 값이라고 NPE 오류가 뜨고
snapshotFlow 도 init 안에서 제대로 동작을 안하네요 (시점이 빨라서 userState 가 초기값일 때 실행되는 거 일수도?)
왜 인지는 모르겠지만 느리게 생성되나 봅니다. (찾아봐도 잘 안나오고 대부분 launchedEffect 를 추천해줍니다)
그래서 LaunchedEffect 안에는 init 이 모두 들어가야 할 거 같고
snapshotFlow 로 만들면
LaunchedEffect(Unit) { // userState NPE error in viewModel init{}; Flow is null
viewModel.getDining()
snapshotFlow { userState }
.collect { state ->
if(!state.isAnonymous) {
viewModel.getShowBottomSheetValue()
viewModel.getNotificationPermissionInfo()
}
}
}
처럼 바뀔 거 같습니다.
근데 LaunchedEffect(userState) 랑 큰 차이는 없을 거 같네요.
kongwoojin
left a comment
There was a problem hiding this comment.
고생하셨습니다.
toolbar height는... 되돌리셔도 될 것 같고 저것들만 확인 후 머지해주세요
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| Text( | ||
| text = "${dining.kcal}kcal", |
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.spacedBy(12.dp) | ||
| ) { | ||
| Text( |
There was a problem hiding this comment.
그냥 궁금한건데 이 부분은 컴포넌트화가 힘든 부분일까요?
중복 코드를 줄이는게 가능해보입니다
There was a problem hiding this comment.
해당 Row 부분 자체를 Item 과 ItemOriginal 을 공통으로 사용중이니 DiningItemMenu 로 묶겠습니다.
Componentization Dining Item menu
Change init launchedeffect key is Unit
Feat Ktlint


PR 개요
이슈 번호: #992
PR 체크리스트
작업사항
작업사항의 상세한 설명
(위젯 진입, 카카오톡 공유 발송,진입 전부 구현 완, AB 테스트 구현 완)
스크린샷
추가
내용