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.

Sign up for GitHub

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

Jump to bottom

Navigation 및 기초 Screen 추가 #24

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Navigation 및 기초 Screen 추가 #24

merged 1 commit into from
Sep 23, 2024

Conversation

ing03201
Copy link
Member

[Issue] #20
[Descriptions]

  • Screen 간 이동 Navigation 내용 추가
  • home / camera / frame / share / setting screen 기초 스크린 생성

@ing03201 ing03201 requested review from DokySp and fetiu September 22, 2024 12:06
@ing03201 ing03201 self-assigned this Sep 22, 2024
@ing03201 ing03201 added the New Implementation New feature label Sep 23, 2024
@ing03201 ing03201 added this to the Phase 1 milestone Sep 23, 2024
@ing03201 ing03201 linked an issue Sep 23, 2024 that may be closed by this pull request
Nav graph 구현 #20
Closed
@DokySp DokySp self-requested a review September 23, 2024 09:16
DokySp
DokySp previously approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@DokySp DokySp left a comment

Choose a reason for hiding this comment

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

LGTM 👍 고생하셨습니다 ㅎㅎ

presenter/src/main/java/com/foke/together/presenter/MainActivity.kt Show resolved Hide resolved
presenter/src/main/java/com/foke/together/presenter/navigation/NavRoute.kt Show resolved Hide resolved
@DokySp DokySp modified the milestones: Phase 1, Phase 2 Sep 23, 2024
fetiu
fetiu previously approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@fetiu fetiu left a comment

Choose a reason for hiding this comment

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

LGTM! 수고 하셨습니다. 상세 내용은 차차 맞춰가면 될 것 같습니다!

@ing03201 ing03201 requested review from DokySp and fetiu September 23, 2024 10:28
fetiu
fetiu approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@fetiu fetiu left a comment

Choose a reason for hiding this comment

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

윤수가 요청해서 @DokySp의 리뷰 이전에 먼저 Approve합니다!

아마 기존의 문제가 윤수 커밋 -> 도균 hilt커밋 머지 순서로 최신 변경사항을 가져오는 방식으로 윤수가 이 PR을 만들어서 생겼던 것 같은데, 도균 hilt커밋 -> 윤수 커밋 리베이스 순서로 바꾸어 해결한 것으로 이해했습니다. 이 과정에서 아마 누락(?) 된 것처럼 보이는 하나의 변경사항을 제외하고 이전 리뷰 시점에서 바뀐 것이 없어 큰 문제가 없을 듯 합니다.

presenter/src/main/java/com/foke/together/presenter/navigation/NavRoute.kt Show resolved Hide resolved
presenter/src/main/java/com/foke/together/presenter/screen/HomeScreen.kt
horizontalAlignment = Alignment.CenterHorizontally) {
Text("Home Screen", fontSize = 40.sp)

Button(onClick = { navigateToCamera("camera") }){
Copy link
Member

Choose a reason for hiding this comment

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

사소한 질문: 이 "camera"는 왜 소문자로 시작하나요? 아래의 "Setting"은 둘 모두 대문자로 시작하는데 이 경우만 일관되지 않길래 하는 가벼운 질문입니다!
아닐 가능성이 높지만, 안드로이드 시스템을 잘 몰라서 이게 혹시라도 버그가 될까 싶기도 해서요:)

Copy link
Member

Choose a reason for hiding this comment

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

오.. 예리한거 같네요 ㅎㅎ
동작상 문제는 없어보이지만, Setting을 setting으로 바꿔야 할 것 같기는 합니다. 둘의 차이가 궁금하기는 하네요!
중요한 문제가 아니라면 굳이 이번에 수정하지 않아도 될 것 같네요 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

넵 다음 Screen UI 구현부에서 수정하도록하겠습니다

presenter/src/main/java/com/foke/together/presenter/screen/HomeScreen.kt Show resolved Hide resolved
DokySp
DokySp reviewed Sep 23, 2024
View reviewed changes
presenter/src/main/java/com/foke/together/presenter/screen/HomeScreen.kt
horizontalAlignment = Alignment.CenterHorizontally) {
Text("Home Screen", fontSize = 40.sp)

Button(onClick = { navigateToCamera("camera") }){
Copy link
Member

Choose a reason for hiding this comment

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

오.. 예리한거 같네요 ㅎㅎ
동작상 문제는 없어보이지만, Setting을 setting으로 바꿔야 할 것 같기는 합니다. 둘의 차이가 궁금하기는 하네요!
중요한 문제가 아니라면 굳이 이번에 수정하지 않아도 될 것 같네요 ㅎㅎ

presenter/src/main/java/com/foke/together/presenter/screen/FrameScreen.kt
Column (
modifier = Modifier.fillMaxWidth(),
horizontalAlignment = Alignment.CenterHorizontally) {
Text("Frame Screen", fontSize = 40.sp)
Copy link
Member

Choose a reason for hiding this comment

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

사소하긴 하지만 여기도 Frame Screen 이 아니라 Share Screen 이어야 할 것 같습니다 😄
굳이 이번에 수정하지 않아도 될 것 같네요 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 내용은 단순 스크린 랜더링 테스트용 아이콘들이어서 삭제할 내용이었습니다.
Screen 작업시에 삭제하겠습니다

presenter/src/main/java/com/foke/together/presenter/screen/ShareScreen.kt
Text("Share Screen", fontSize = 40.sp)
Button(onClick = { popBackStack }){
Text("Home")
}
Copy link
Member

Choose a reason for hiding this comment

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

여기도 아래와 비슷한 맥락에 코드가 있으면 좋을 것 같기는 합니다

navigateToCamera("camera"

사소한 것 같아 굳이 이번에 수정하지 않아도 될 것 같네요 ㅎㅎ

@DokySp DokySp self-requested a review September 23, 2024 13:19
@ing03201 ing03201 merged commit 3f128c5 into FoKE-Developers:main Sep 23, 2024
1 check passed
@DokySp DokySp added the feature/UI feature-ui label Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fetiu fetiu fetiu approved these changes

@DokySp DokySp Awaiting requested review from DokySp

Assignees

@ing03201 ing03201

Labels
feature/UI feature-ui New Implementation New feature
Projects
None yet
Milestone
Phase 2
Development

Successfully merging this pull request may close these issues.

Nav graph 구현
3 participants
@ing03201 @DokySp @fetiu