Skip to content
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

[UI/YAF-68] 알람 설정 시 요일 반복 설정 #34

Merged
merged 17 commits into from
Jan 14, 2025

Conversation

DongChyeon
Copy link
Member

@DongChyeon DongChyeon commented Jan 13, 2025

Related issue 🛠

closed #29

어떤 변경사항이 있었나요?

  • 🐞 BugFix Something isn't working
  • 🎨 Design Markup & styling
  • 📃 Docs Documentation writing and editing (README.md, etc.)
  • ✨ Feature Feature
  • 🔨 Refactor Code refactoring
  • ⚙️ Setting Development environment setup
  • ✅ Test Test related (Junit, etc.)

CheckPoint ✅

PR이 다음 요구 사항을 충족하는지 확인하세요.

  • PR 컨벤션에 맞게 작성했습니다. (필수)
  • merge할 브랜치의 위치를 확인해 주세요(main❌/develop⭕) (필수)
  • Approve된 PR은 assigner가 머지하고, 수정 요청이 온 경우 수정 후 다시 push를 합니다. (필수)
  • BugFix의 경우, 버그의 원인을 파악하였습니다. (선택)

Work Description ✏️

  • BaseViewModel의 currentState에 접근하여 현재 상태값을 가져올 수 있도록 함
  • OrbitPickerItem의 현재 값을 부모 composable에서 관리하도록 함
  • 알람 설정 화면의 요일 설정 로직을 구현함
  • OrbitPicker에서 시간이 한 바퀴 회전 시 AM/PM 이 바뀌도록 수정함
default.mp4

Uncompleted Tasks 😅

default.mp4

위의 영상을 보면 분이 52 -> 51로 넘어가는 과정에서 시간이 6이 선택되는 것을 확인할 수 있음

OrbitPicker, OrbitYearMonthPicker에서 OrbitPickerItem 스크롤 시 다른 OrbitPickerItem에도 영향을 줌

  • 선택한 값을 부모 composable에서 관리하는 과정에서 선택한 값이 바뀔 때마다 recomposition
  • 이 때, OrbitPickerItem의 처음 스크롤 계산 로직으로 인해, 스크롤 상태가 바뀌면서 onValueChange가 호출되는 것으로 추정 중...

To Reviewers 📢

@Composable
fun AlarmAddEditRoute(
    viewModel: AlarmAddEditViewModel = hiltViewModel(),
) {
    val state by viewModel.container.stateFlow.collectAsStateWithLifecycle()
    val sideEffect = viewModel.container.sideEffectFlow

    LaunchedEffectWithLifecycle(sideEffect) {
        sideEffect.collect { effect ->
            when (effect) {
                is AlarmAddEditContract.SideEffect.NavigateBack -> {
                }

                is AlarmAddEditContract.SideEffect.Navigate -> {
                }
            }
        }
    }

    AlarmAddEditScreen(
        state = state,
        processAction = viewModel::processAction,
    )
}

onboarding 모듈쪽을 살펴보면 stateProvieder()를 통해서 상태를 주입하도록 되어 있는데
해당 과정 없이 바로 state를 넘겨줘도 되지 않을까 싶어서 이렇게 구현했습니다.
특별한 이유 있으시면 코멘트 달아주세요!

그리고 네이밍도 일단은 state, processAction으로 하였습니다.

@DongChyeon DongChyeon added 📱 UI 사용자 인터페이스 관련 작업 💪 동현동현동현동현동현 labels Jan 13, 2025
@DongChyeon DongChyeon self-assigned this Jan 13, 2025
@DongChyeon DongChyeon requested a review from MoonsuKang January 14, 2025 06:46
Copy link
Member

@MoonsuKang MoonsuKang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어려분 부분이 많았는데 수고하셨습니다!
그리고 stateProvider()를 사용한 이유를 남깁니다!
상태 주입 방식을 추상화하여, 테스트 코드 작성이나 다른 의존성 주입 시 유연성이 증가한답니다!
음. 예를들어서 ViewModel 없이 특정 상태를 하드코딩 하거나, 외부에서 상태를 직접 주입해야 하는 경우를 대비할 수 있는 구조를 작성한 것이죠!
그러면 재사용성 또한 증가하겠죠? 계속 val state = viewmodel~ 을 선언할 필요가 사라지니?
이렇게 하면 Preivew에서도 viewmodel호출 안해도 된다는 장점이 있답니다!
이 구조는 상의를 좀 더 해봅시다~

Comment on lines 64 to 68
fun AlarmAddEditScreen(
state: AlarmAddEditContract.State,
processAction: (AlarmAddEditContract.Action) -> Unit,
) {
Column(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p4
inspector 로 테스트는 해보지 않았지만, 현재 이 구조면 state가 변경되면 Column 전체가 recomposition 되는 구조일 수도 있을 것 같은데 혹시 아닌가요???

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
image

하위 composable에 필요한 값만 전달해서 recomposition 횟수 최적화

Comment on lines 64 to 66
val updatedDays = currentState.selectedDays.toMutableSet().apply {
if (contains(day)) remove(day) else add(day)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p4
혹시 toMutableSet을 사용한 이유가 있으실까요?
Set으로 처리할 수도 있을 것 같아서요!
toMutableSet는 아시다시피 항상 새로운 데이터를 생성하고 그것을 복사하는 구조 잖아요? 그러면 메모리 사용적인 측면에서 비효율적일 수 있다고 생각해서요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val updatedDays = if (day in currentState.selectedDays) {
    currentState.selectedDays - day
} else {
    currentState.selectedDays + day
}

수정했습니다

Comment on lines 17 to 21
object HomeRoute {
const val HOME = "home"

const val ALARM_ADD_EDIT = "alarm_add_edit"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p3
나중에 core module에서 object{object}로 관리 기억하기

Comment on lines +39 to +44
updateState {
copy(
isWeekdaysChecked = isChecked,
selectedDays = updatedDays,
)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p4
요 구조면 저 상태를 관찰하고 있는 composable에서 중복된 recomposition 문제가 발생하지 않을까란 생각이 드네용

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요거 제가 이해를 잘못했던거 같습니다! 서로 의존관계에 있다는 것을 확인을 못했네요!

Copy link
Member

@MoonsuKang MoonsuKang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다~!!

@DongChyeon DongChyeon merged commit 62db29a into develop Jan 14, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 동현동현동현동현동현 📱 UI 사용자 인터페이스 관련 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UI] 알람 설정 시 요일 반복 설정
2 participants