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

DEP-51 feat: Navigator 구현 #17

Merged
merged 8 commits into from
Oct 17, 2022
Merged

Conversation

juhwankim-dev
Copy link
Member

💁‍♂️ PR 내용

📢 전달사항

  1. 혜인님 레포에는 BottomNavigationView를 TabLayout이랑 ViewPager로 만들었던데
    뭔가 특정 커스텀을 위해 그렇게 만드신 것 같아서
    저는 그냥 BottomNavigationView 사용했습니다! (Jetpack 아님)
    근데 제가 Navigator를 제대로 이해하고 사용한 건지 모르겠네요...
    만들어 두긴 했는데 활용이 안되고 있는 것 같은데 😵‍💫
    우선 UT 전 결과물에는 문제 없습니다.

  2. StatusBar를 흰색으로 변경했습니다.
    이거 하나를 위해 브랜치를 따기 애매해서 같이 바꿔버렸어요.
    그래도 따로 하나 파는 게 바람직 한 걸까 싶기도 하고...

  3. BaseFragment의 _binding 부분이 계속 오류가 떠서 코드를 조금 수정했습니다.

@juhwankim-dev juhwankim-dev self-assigned this Oct 16, 2022
@juhwankim-dev juhwankim-dev requested a review from a team as a code owner October 16, 2022 15:43
@juhwankim-dev
Copy link
Member Author

SonarCloud Code Analysis에서 Quality failed가 뜬 이유는
제가 todo 주석 달아 놔서 그렇고 -> 이건 혜인님한테 말씀드리기 위한 주석이였음
메서드 하나 만들어 놓고 사용을 안 해서 그렇습니당 -> 이건 제가 다음 브랜치에서 사용할 예정...

}

private fun changeFragment(fragment: Fragment) {
supportFragmentManager.beginTransaction().replace(binding.flMain.id, fragment).commit()
Copy link
Member

Choose a reason for hiding this comment

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

여기도 transaction 이 있네요. 신기하네요 ㅋㅋ

Copy link
Member Author

Choose a reason for hiding this comment

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

flutter에도 있나보군요! ㅋㅋㅋ

<item
android:id="@+id/homeFragment"
android:icon="@drawable/ic_tab_home"
android:title="홈" />
Copy link
Member

Choose a reason for hiding this comment

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

title 값은 string.xml 에 정의하는 것보다 bottom_nav_menu.xml 에서 관리하는게 나을까요?

Copy link
Contributor

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.

노란색 경고 뜨긴합니다... ㅎㅎ...
정석은 이런 거 다 string.xml에 놓는 게 맞긴 한데
저번에 oo회사에서는 이런 걸 정말 다 일일이 string.xml에 넣어 놓고 사용하나 봤는데
그냥 이렇게 짜는 것 같더라구요 🤔

Copy link
Contributor

@kimhyeing kimhyeing left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~! 구조 짜주셔서 감사해요!!!

@@ -0,0 +1 @@
/build
Copy link
Contributor

Choose a reason for hiding this comment

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

image
현재 위와 같이 빨간색 동그라미가 보이는데요, 여기에 마우스를 가져다대면 No newline at end of file이라고 되어있습니다! 머지하는 과정에서 파일 끝에 개행이 있으면 오류가 날 가능성이 있다고 하더라구요 (근데 오류가 나는걸 본 적이 없긴 합니다 ㅎㅎ)

아래는 안드로이드 스튜디오에서 파일 개행추가를 자동으로 설정해주는 방법과 그 이유입니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

엇 친절하게 설명과 링크까지... 감사합니다 ㅎㅎ!

Comment on lines +15 to +16
// 혜인님 레포에 있는 것처럼 setOnSingleClickListener를 만들어서 사용할 지 여부와
// viewModel에서 클릭리스너를 달아줄지 등을 협의하고 확정되면 추후에 수정하겠습니다.
Copy link
Contributor

Choose a reason for hiding this comment

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

viewModel에서 클릭리스너를 다는 방식은 어떤건가용 ?.?

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 에서 onClick에 람다식 넣어주는 거요!

Copy link
Contributor

@kimhyeing kimhyeing Oct 17, 2022

Choose a reason for hiding this comment

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

아하!! 흠 그러게요,,.. 섞어서 써도 괜찮을 것 같고 그냥 하나로 정하는 것도 좋을 것 같아요!
서버 호출 생각하면 setOnSingleClickListener를 쓰는게 좋을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

setOnSingleClickListener 이거 interval을 주고 그러던데 정확히 무엇을 위한 커스텀인가요? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 굳이 interval 값을 고정하는게 아니라 파라미터로서 주는 이유는 잘 모르겠습니닷.....

Comment on lines 54 to 56
implementation deps.jetpack.appCompat
implementation deps.jetpack.material
implementation deps.jetpack.constraintLayout
Copy link
Contributor

Choose a reason for hiding this comment

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

implementation(jetpackDeps)를 했는데 오류가 뜨는건가요??

Copy link
Member Author

Choose a reason for hiding this comment

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

앗... 제가 헷갈렸네요... 아닙니다 ㅠ 롤백해두었습니다.

@juhwankim-dev juhwankim-dev merged commit 5cc4ca4 into develop Oct 17, 2022
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@juhwankim-dev juhwankim-dev deleted the feature/DEP-51_navigator branch October 17, 2022 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants