-
Notifications
You must be signed in to change notification settings - Fork 9
[Feat/#1447] 스프린트 3 앱잼팀 랭킹, 앱잼팀 미션 뷰 구현 #1448
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
Conversation
Hyobeen-Park
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
왕 수고하셨습니다!!
코드를 보니 서버에서 받아와야 하는 데이터들은 대부분 하드코딩하신 것 같아요!! 이렇게 하면 나중에 서버 연결 할 때 UI단 스크린/컴포넌트까지 전부 다 수정해야 하는 불편함이 있습니다ㅎㅎ 필수는 아니지만 컴포넌트 설계 후 미리 슬롯 뚫어서 완전한 상태의 스크린을 만드는 방식으로 구현해보는 것도 좋을 것 같습니다!! 이렇게 하면 서버 연결 할 때 뷰모델 + 가장 상위 컴포(라우트)만 수정하면 되거든요😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
랭킹은 배경만 추출하고 순위는 텍스트로 해야 하지 않을까요?? 앞으로 계속 사용한다면 앱잼 팀 수가 기수마다 달라지는 문제도 있고 무엇보다 현재 기수에도 총 11팀 있는 걸로 알고 있어서요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앱잼 팀 수가 기수마다 달라지는 문제도 있고 무엇보다 현재 기수에도 총 11팀 있는 걸로 알고 있어서요!
이걸 생각을 못했네요. 말 하신 방법이 맞네요 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서버 api 이름이 앱잼팀 오늘의 득점 랭킹 TOP10 조회하기라서 확인을 해봐야 할 것 같네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api 이름과는 별개로 쿼리로 보내는 size값에 맞게 순위 보여주고 있어서 기획쌤들 의도에 맞게 구현하시면 될 것 같습니다! 슬랙에 질문 업로드 하신 것 같아서 이건 슬랙으로 확인할게요!
| // TODO - 서버 응답 -> UrlImage 사용 | ||
| // UrlImage( | ||
| // url = "", | ||
| // contentDescription = null | ||
| // ) | ||
|
|
||
| // TODO 임시 이미지 | ||
| Image( | ||
| imageVector = ImageVector.vectorResource(id = org.sopt.official.designsystem.R.drawable.img_fake_red), | ||
| contentDescription = null, | ||
| contentScale = ContentScale.Crop, | ||
| modifier = Modifier.fillMaxSize() | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 서버 연결 후에 수정되는걸까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 서버 연결 후 삭제할 예정입니다
| ) { | ||
| if (true) { // TODO - 프로필 이미지 존재 여부 | ||
| Icon( | ||
| imageVector = ImageVector.vectorResource(id = R.drawable.ic_empty_profile), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty_profile 말고 ic_user_profile 이게 맞는 것 같은데 한 번 더 확인 부탁드려요😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
라우트랑 스크린 파일을 분리하신 이유가 궁금해요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
나중에 NavGraphBuilder.AppjamtapGraph 같은 네비게이션 파일에서 쓰려고 했는데 복잡한 뷰가 아니면 route가 없어도 될 것 같아요 😊
이건 좀 더 보고 나중에 필요 없으면 Screen만 남기도록 할게요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 Route를 만든 이유가 아닌 파일을 분리한 이유가 궁금했어요..!! 저는 웬만하면 route랑 screen은 한 파일에 두는 편인데 다른 방식도 궁금해서요ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
! 아 그러네요.. 저도 코드가 너무 길어지는 경우가 아니면 항상 한 파일에 둘 다 넣어서 만드는데 이건 잘못한 것 같네요
한 파일에 들어가도록 수정해두겠습니;다 @Hyobeen-Park
| ) { | ||
| repeat(times = 3) { | ||
| TopRankingTeamMission( | ||
| modifier = Modifier.width(topRankingItemWidth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 저라면 width를 고정으로 지정했을 것 같은데..!! 성민오빠는 화면 크기에 대한 비율로 정의하셨네요!! 이유가 궁금해요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 처음에는 143.dp로 고정된 수치를 줬는데, 피그마가 전체 width 375dp 대비 143.dp 기준이라 여러 기기에서 화면 대비 일정한 비율을 유지하려면 상대적 크기로 하는 게 나을 것 같아서 이렇게 구현했어요. 혹시 고정 dp가 더 적절할 것 같으신가요?
| Row( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .align(Alignment.Start) | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요기도 정렬 필요해보여요!!
| Box( | ||
| modifier = modifier | ||
| .clip(shape = RoundedCornerShape(size = 10.dp)) | ||
| .background(color = SoptTheme.colors.onSurface900) | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 Box 없이도 구현 가능해보이네요!
| onBackButtonClick = {}, | ||
| modifier = Modifier | ||
| .padding(vertical = 12.dp) | ||
| .padding(start = 16.dp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 start 20.dp인듯요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이게 피그마에 start 수치가 2개 있는데 하나는 20.dp, 하나는 16.dp로 되어있어요.
다른 앱잼탬프 화면들의 탑바 화살표의 start padding은 모두 16.dp로 되어있어서 16으로 했어요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
슬랙으로 확인했는데 16이 맞다고 하시네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 슬랙 확인했습니다~!
| } | ||
|
|
||
| @Composable | ||
| fun DescriptionText( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
접근제한자 붙여주세요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실수입니다 ㅎㅎ
| ) | ||
| } | ||
|
|
||
| // TODO - 앱잼탬프 홈 화면에서도 사용되는 컴포넌트 -> 추후 이 컴포넌트 삭제하고 공용 컴포넌트 사용해야 함 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 갑자기 든 생각인데요!! 이거 앱잼탬프 홈이랑 거의 똑같은데 같은 스크린 내에서 분기처리 해야할지 그리드만 컴포넌트로 빼야할지 살짝 고민되네요..!! 미션 리스트 보여주는 뷰라 같은 스크린으로 봐야 맞는 것 같기두 하구.. 흐음...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이게 조금 애매한 것 같은데 제 생각에는 분리하는게 나을 것 같은데.. 제가 앱잼탬프 홈을 구현해보지 않아서 모르겠네요
@sonms 어떻게 생각하시나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이게 조금 애매한 것 같은데 제 생각에는 분리하는게 나을 것 같은데.. 제가 앱잼탬프 홈을 구현해보지 않아서 모르겠네요 @sonms 어떻게 생각하시나요??
저도 분리가 나을 것 같아요 if 문으로 보여질 컴포넌트들을 제어하기 보다는 다른 기능으로 보고 처리하는게 나아보이네요
좋은 의견 감사합니다 :) |
Related issue 🛠
Work Description ✏️
Screenshot 📸
default.mp4
default.mp4
To Reviewers 📢
MissionsGridComponent 이 컴포넌트는 앱잼탬프 홈에도 쓰이는 컴포넌트라서 민성이 코드 머지하면 삭제 후 해당 컴포넌트로 사용할 예정입니다.
앱잼팀 미션 뷰 실행영상은 preveiw 입니다. (바텀 네비게이션바 있다고 가정하고 봐주세요)