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

[Feature/#9] : 약속 생성 UI 구현 #12

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

twogarlic
Copy link

✅ 𝗖𝗵𝗲𝗰𝗸-𝗟𝗶𝘀𝘁

  • merge할 브랜치의 위치를 확인해 주세요.(main❌/develop⭕)
  • 리뷰가 필요한 경우 리뷰어를 지정해 주세요.
  • 리뷰는 PR이 올라오면 최대한 빠르게 진행합니다.
  • P1 단계의 리뷰는 빠르게 확인 후 반영합니다.
  • Approve된 PR은 assigner가 머지하고, 수정 요청이 온 경우 수정 후 다시 push를 합니다.

📌 𝗜𝘀𝘀𝘂𝗲𝘀

📎 𝗪𝗼𝗿𝗸 𝗗𝗲𝘀𝗰𝗿𝗶𝗽𝘁𝗶𝗼𝗻

  • 약속 정보 입력
  • 기간 입력

📷 𝗦𝗰𝗿𝗲𝗲𝗻𝘀𝗵𝗼𝘁

Screen_recording_20250103_003255.mp4

💬 𝗧𝗼 𝗥𝗲𝘃𝗶𝗲𝘄𝗲𝗿𝘀

... .. ㄹㅇ 우당탕탕 와르르멘션인데 저 어떡함요 ?ㅠ
https://www.youtube.com/watch?v=r2ko422xW0w&t=877s

@twogarlic twogarlic self-assigned this Jan 2, 2025
@twogarlic twogarlic requested a review from a team as a code owner January 2, 2025 15:40
Copy link
Contributor

@youjin09222 youjin09222 left a comment

Choose a reason for hiding this comment

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

와우 너무 고생 많으셨습니다! 👏👏👏

Copy link
Contributor

@youjin09222 youjin09222 Jan 2, 2025

Choose a reason for hiding this comment

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

P2: 변경되지 않는 텍스트들은 추출해주시면 좋을 것 같습니다!

Copy link
Contributor

@gaeulzzang gaeulzzang left a comment

Choose a reason for hiding this comment

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

외모췌크 말구 ktlintCheck 해주세요!!!! 헿
스트링 추출도!!!!

import com.sopt.presentation.R

@Composable
fun DetailScreen(onNextClick: () -> Unit, onBackClick: () -> Unit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

P1: Route안에서 Screen을 호출해야돼욥 ㅠㅠ GroupDetailRoute 확인하시면서 구조 바꿔주세욤

Copy link
Member

Choose a reason for hiding this comment

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

P1 : 지금 저희 프로젝트 구조에서는 ~Route에는 viewModel을 포함한 프로세스 처리 로직들을 담는 방식입니다. ~Screen은 최소의 프로세스 로직을 포함하여 UI를 띄우기 위한 역할로 이해하시면 될 것 같아요. xml 파일에 해당하는거죠. 그러므로 역할 구분을 더 명확히 하여 코드를 짜면 좋을 것 같아요

UI를 제외한 프로세스 로직이 필요하지 않는 단순 화면이거나 재사용되는 화면의 본체 파일이라면 괜찮겠지만 각 페이지마다 route-screen이 짝으로 필요하다고 이해하시면 쉬울 것 같아요. 물론 상황에 따라서 달라지겠지만.. 만약 코드가 너무 길어져서 분리하고 싶다면 ~Route 파일과 ~Screen 파일로 분리하는건 상관없습니다!

말이 길어졌지만 어쨋든.. 지금 상황에서는 CalendarDetailRoute 파일을 생성하여 그 안이든 따로이든 CalendarDetailScreen 코드를 작성하면 되실 것 같네요~

하나 덧붙이자면 모든 파일, 변수, 함수 등의 네이밍은 해당 이름만 봤을 때 어떤 역할을 하는 아이인지 알 수 있게끔 붙여주세요. 추후 프로젝트의 규모가 커지거나 시간이 지났을 때 좋은 네이밍으로 지었어야 코드를 짠 사람도, 협업하는 사람들도 쉽게 이해할 수 있을테니까요!

.padding(horizontal = 16.dp, vertical = 24.dp),
topBar = {
NoostakTopAppBar(
title = "약속 만들기",
Copy link
Contributor

Choose a reason for hiding this comment

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

P1: 스트링 추출띠예우예

Comment on lines 69 to 80
Image(
painter = painterResource(
id = if (isSingleDateMode) R.drawable.ic_calendar_toggle_on
else R.drawable.ic_calendar_toggle_off
),
contentDescription = null,
contentScale = ContentScale.FillBounds,
modifier = Modifier
.clickable {
isSingleDateMode = !isSingleDateMode
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

P1: 이미지 넣지 말구 compose에서 지원하는 switch 컴포저블로 변경띠

Spacer(modifier = Modifier.weight(1f))

NoostakButton(
text = "다음",
Copy link
Contributor

Choose a reason for hiding this comment

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

P1: extract string plz

core/src/main/res/drawable/ic_calendar_back.xml Outdated Show resolved Hide resolved
core/src/main/res/drawable/ic_calendar_front.xml Outdated Show resolved Hide resolved
val timeValue = newValue.toIntOrNull() ?: 0
viewModel.updateTime(timeValue)
},
modifier = Modifier.width(50.dp),
Copy link
Contributor

Choose a reason for hiding this comment

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

P1: width가 너무 적어서 클릭 영역이 좁아요 ㅠ "시간" 텍스트를 제외한 나머지 영역을 모두 텍스트 필드로 줍시답

Copy link
Member

Choose a reason for hiding this comment

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

P1 : 디쟌 쌤에게 터치 영역 물어보는게 좋을 것 같아요. GUI에도 애매

Comment on lines 134 to 137
onValueChange = { newValue ->
val timeValue = newValue.toIntOrNull() ?: 0
viewModel.updateTime(timeValue)
},
Copy link
Contributor

@youjin09222 youjin09222 Jan 2, 2025

Choose a reason for hiding this comment

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

P1 :숫자를 여러개 입력하면 필드 영역이 작아서 숫자가 잘려 보이는데
영역은 넓게하고, if문 사용해서 글자 수 제한하는게 더 좋을 것 같습니다!

Suggested change
onValueChange = { newValue ->
val timeValue = newValue.toIntOrNull() ?: 0
viewModel.updateTime(timeValue)
},
onValueChange = {
if (it.length <= maxLength) {
onTextChange(it)
}
}

Copy link
Member

Choose a reason for hiding this comment

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

P1 : 기디 연락 ㄱㄱ

Copy link
Member

@Eonji-sw Eonji-sw left a comment

Choose a reason for hiding this comment

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

캘린더 구현할 때 고려해야하는게 많아서 힘드셨을텐데 열심히 하셨네용!! NoostakCalendar 파일의 경우 수정해야할 것이 많아보이는데 우선 UI 코드 위주로만 봤습니다! 코리들 꼼꼼히 읽어서 반영하시고 다시 노티 주시면 좋을 것 같아요. 짧은 기간 안에 잘해주셨어요!!! 글로 줄줄 써서 읽기 싫겠지만 도움은 될거예용 ㅜㅜ..

힘들테니 제 짝남 웃는거 보고 홧팅
image

Comment on lines 134 to 137
onValueChange = { newValue ->
val timeValue = newValue.toIntOrNull() ?: 0
viewModel.updateTime(timeValue)
},
Copy link
Member

Choose a reason for hiding this comment

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

P1 : 기디 연락 ㄱㄱ

val timeValue = newValue.toIntOrNull() ?: 0
viewModel.updateTime(timeValue)
},
modifier = Modifier.width(50.dp),
Copy link
Member

Choose a reason for hiding this comment

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

P1 : 디쟌 쌤에게 터치 영역 물어보는게 좋을 것 같아요. GUI에도 애매

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UI] : 약속 생성 UI
4 participants