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

#111 Home 진입 이후 플로우 Compose Navigation 마이그레이션 작업 #127

Merged
merged 114 commits into from
Jul 11, 2023

Conversation

JaesungLeee
Copy link
Member

@JaesungLeee JaesungLeee commented Jul 10, 2023

작업 내역

  • 홈 진입 이후 플로우 XML Navigation -> Compose Navigation으로 마이그레이션
  • xml navigation graph 삭제 및 관련 Activity, Fragment 제거
    • graph_navigation
    • graph_signal
    • graph_home
    • etc...

To. Reviewer

  • 현재 우리 서비스 전체 플로우에서 온보딩 이후 홈 진입 플로우에 대해서 마이그레이션 완료했습니다.

  • 중점적으로 보실 부분은 KeyLinkAppState랑 KeyLinkNavHost 부분입니다~

  • KeyLinkAppState

    • 앱 전반적인 상태를 관리합니당 remember도 하고있구여 + 추후에 네트워크 상태 감지같은 부분들도 여기에 들어가야 합니다~
  • KeyLinkNavHost

    • 홈화면에 진입했을 때 모든 그래프들이 그려지는 컨테이너입니다~
  • 패키지를 좀 분리해놨습니당~ ( presentation / feature / ... )

  • 로그인, 온보딩에 대한 마이그레이션은 급한 작업들 먼저 해놓고 진행 예정입니다~ + 같은 이슈로 진행할 예정입니다~

  • 모든 작업들은 NowInAndroid 참고했습니당

  • XML Navigation에서 사용되던 파일들 및 기존 Navigation 의존성은 남겨뒀고 마지막에 제거할 예정입니당

관련 이슈

close #

* bottomBar
* snackbarhost에 KeyLinkSnackbar 연결
* NavHost
* appState
…to feature/navigation-migration

# Conflicts:
#	presentation/src/main/java/com/mashup/presentation/KeyLinkApp.kt
#	presentation/src/main/java/com/mashup/presentation/feature/home/navigation/HomeNavigation.kt
#	presentation/src/main/java/com/mashup/presentation/feature/subscribe/SubscribeKeywordScreen.kt
#	presentation/src/main/java/com/mashup/presentation/onboarding/OnBoardingViewModel.kt
#	presentation/src/main/res/layout/fragment_notification_permission_guide_compose.xml
#	presentation/src/main/res/navigation/graph_signal.xml
@JaesungLeee JaesungLeee self-assigned this Jul 10, 2023
@JaesungLeee JaesungLeee added the Refactor 리팩토링 관련 label Jul 10, 2023
@JaesungLeee JaesungLeee changed the title #111 Home 화면 Compose Navigation 마이그레이션 작업 #111 Home 진입 이후 플로우 Compose Navigation 마이그레이션 작업 Jul 10, 2023
Copy link
Collaborator

@Huijiny Huijiny left a comment

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋㅋ온보딩뷰모델 diff에 추가되어있는거 웃기다
고생했으 재성
같이 리뷰합시다~~

Copy link
Collaborator

@Huijiny Huijiny left a comment

Choose a reason for hiding this comment

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

고생했다 재성이
확실히 설명듣고 보니까 더 잘보인다(여전히 살짝 어렵지만)

* 4. NetworkMonitor(isOffline / isOnline)
* 5. ...
*/
@Stable
Copy link
Collaborator

Choose a reason for hiding this comment

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

stable..사용하셧네요 결국 크크👏
어떤게 좋다고 합니까?

Copy link
Member Author

Choose a reason for hiding this comment

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

리컴포지션이 일어나는 대표적인 조건이 전달되는 파라미터 값이 변경될 때인데, KeyLinkAppState를 KeyLinkApp에 전달할 때 @stable을 통해 컴포저블 함수를 다시 그리지 않고 Skippable하게 유지할 수 있습니당
KeyLinkAppState는 일반 클래스이기 때문에 컴파일러가 Stable함을 추론할 수 없고 명시적으로 skip할 수 있도록 @stable을 붙이구용

onReportIconClick: () -> Unit,
onReplyButtonClick: () -> Unit
) {
navigation(
Copy link
Collaborator

Choose a reason for hiding this comment

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

navigation에 가장 상위 그래프?를 정의하고 하위에 그 그래프 안에있는 컴포저블(XXRoute)를 연결해주는 느낌이구나

Copy link
Member Author

Choose a reason for hiding this comment

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

원래는 그냥 composable로도 NavGraph를 만들 수 있는데, navigation하위에 두게 되면 NestedGraph형태로 만들 수 있다고 하네용.. 나두 이정도로 이해해서 navigation으로 래핑하게 되면 NestedGraph 형태로 만들게 되서 다른 NavGraph에서도 사용할 수 있다 정도로만 이해하고 있습니당~

}


//class HomeActivity : BaseActivity<ActivityHomeBinding>(R.layout.activity_home) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

요기 주석은 혹시몰라서 안지운건가효?

Copy link
Member Author

Choose a reason for hiding this comment

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

넹 요거는 일단 안지웠구, 최종적으로 XML Navigation 날릴때 일괄적으로 제거할 예정입니당~

@Composable
fun KeyLinkNavHost(
appState: KeyLinkAppState,
onShowSnackbar: suspend (String, String?) -> Boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

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

KeyLinkNavHost라고 하면 제너럴하게 앱 전반적인 내용을 관할해야할거같은데 onShowSnackbar만 되게 구체적이라 살짝 어색하넹

Copy link
Member Author

Choose a reason for hiding this comment

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

Scaffold에서 snackBarHostState로 스낵바를 띄워야 해서 저렇게 열어둔거긴해
만약에 화면에서 스낵바를 띄워야한다고 하면 저렇게 스낵바 이벤트를 상위까지 쭉 올려야하는 것 같은데, 스낵바 작업할떄 한번 해보고 추가 코멘트 달아놓을게!!

* @author jaesung
* @created 2023/07/10
*/
fun NavController.navigateToGuideRoute(navOptions: NavOptions? = null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이런 화면 이동 정의는 각 화면 패키지에 navigation 패키지 파서 만들면되는건가?

Copy link
Member Author

Choose a reason for hiding this comment

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

정답~~!!

Copy link
Collaborator

@sooziini sooziini left a comment

Choose a reason for hiding this comment

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

고생하셨슴다 navigation 부분은 설명해줘서 덕분에 어느정도는 이해했지만 머지하고 더 자세히 함 봐볼 예정.ㅎㅎ 로그인/온보딩/홈 KeyLinkApp에 합칠지 따로 가져갈지 정해지면 만약 따로 가져간다면 로그인을 내가 함 바꿔볼까

@JaesungLeee
Copy link
Member Author

@sooziini 로그인 / 온보딩 작업 진행하는거 좋은 것 같아!!
참고한 자료가 있긴한데, 다들 컴포즈 네비게이션 작업할 때 참고하면 좋을 것 같아용~

@JaesungLeee JaesungLeee merged commit e2f757f into mash-up-kr:develop Jul 11, 2023
@JaesungLeee JaesungLeee deleted the feature/navigation-migration branch July 11, 2023 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor 리팩토링 관련
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants