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

[BSVR-82/FEAT] 시야 후기 등록 1차 UI 구현 #12

Merged
merged 25 commits into from
Jul 10, 2024

Conversation

minju1459
Copy link
Collaborator

💻주요 작업 내용

  • 등록 메인 화면 구현
    • 날짜 date picker
    • 사진 등록 시 선택순(1-2-3) 정렬
    • 3번째 사진 따로 추가 시 포커싱
    • 사진 개수 3개 -> (+) 버튼 사라짐
    • 하단 오른쪽 테스트 고려
  • 사진 업로드 바텀 시트 구현
  • 사진 등록 - 사진 촬영 업로드 구현
  • 사진 등록 - photo picker 업로드 구현
20240710_174517.mp4

🎞리뷰 요청 사항

  • 뷰가 많아서 1,2차로 나눠서 pr 올립니다 !
  • 대략적으로 font(size,fontweight,style), color는 적용해놨는데 디자인 시스템 완성본 받으면 한번에 적용하는게 좋을 것 같습니당
  • 사진 카메라로 찍으면 업로드 시 화질이 낮아지는데 좋은 방법이 있을까요?
  • 2차 UI와 이어지는 부분이 있어 디테일적인 부분은 아직 미완성인 부분 감안 부탁드립니당

Sorry, something went wrong.

@minju1459 minju1459 added this to the 1차 스프린트 개발 milestone Jul 10, 2024
@minju1459 minju1459 self-assigned this Jul 10, 2024
Copy link
Collaborator

@SsongSik SsongSik left a comment

Choose a reason for hiding this comment

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

수고많았어~~~! xml 파일들 코드 컨벤션이랑 코드정렬 해주고!

Comment on lines 45 to 46
<LinearLayout
android:id="@+id/btn_take_photo"
Copy link
Collaborator

Choose a reason for hiding this comment

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

id 컨벤션 확인해주세요!

Comment on lines 8 to 10
<DatePicker
android:id="@+id/datePicker"
android:layout_width="wrap_content"
Copy link
Collaborator

Choose a reason for hiding this comment

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

요기도 id 컨벤션!

Comment on lines 79 to 83
for (uri in newImageUris) {
if (!selectedImageUris.contains(uri)) {
selectedImageUris.add(uri)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이미 추가된 이미지 중복 방지하려고 넣은 것 같은데, 가독성 좋게 코틀린스럽게 변경해보면

selectedImageUris.addAll(newImageUris.filterNot { selectedImageUris.contains(it) })
updateImageViews()

이렇게 바꿔볼 수 있겠당 돌려보진 않아서 한 번 더블체크해보고!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

훨씬 좋네용 .. 감사합니다 !!

Comment on lines 90 to 91
for ((index, uri) in selectedImageUris.withIndex()) {
if (index < imageViews.size) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

selectedImageUris.forEachIndexed { index, uri ->

이렇게 바꿔볼 수도 있을 것 같아요 !

Comment on lines 16 to 19
var onDateSelected: ((year: Int, month: Int, day: Int) -> Unit)? = null
private var selectedYear: Int = Calendar.getInstance().get(Calendar.YEAR)
private var selectedMonth: Int = Calendar.getInstance().get(Calendar.MONTH)
private var selectedDay: Int = Calendar.getInstance().get(Calendar.DAY_OF_MONTH)
Copy link
Collaborator

Choose a reason for hiding this comment

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

getInstance() 중복되는데, 하나로 만들고 그거 참조하는게 좋겠다!

Comment on lines 51 to 53
val bundle = Bundle().apply {
putStringArrayList(SELECTED_IMAGES, ArrayList(uriList))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

val bundle = bundleOf(SELECTED_IMAGES to uriList)
이렇게 Pair 형태로 깔끔하게 바꿔 볼 수 있습니당

Copy link
Collaborator

@BENDENG1 BENDENG1 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,9 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

컨벤션이 아직 명확하지 않은거 같은데

``rect_gray50_fill_gray900_line_10```
이런식으로 하면 되려나? (10은 radius) stroke의 width까지 쓰려면 너무 복잡해지려나..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오호 .. 그거까진 컨벤션을 안정했긴 한데 나중에 디자인 시스템 받구 다같이 정해보자 !

@@ -24,6 +28,11 @@
<category android:name="android.intent.category.LAUNCHER" />
</intent-filter>
</activity>

<activity
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
Collaborator Author

Choose a reason for hiding this comment

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

앗 main은 그대로 두고 작업한 액티비티는 추가하는거 아니었나 ?!

android:background="@drawable/rect_white_fill_32">

<View
android:id="@+id/iv_handler"
Copy link
Collaborator

Choose a reason for hiding this comment

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

v_handler라고 해야하나? 애매하네

app:layout_constraintTop_toBottomOf="@id/tv_upload">

<LinearLayout
android:id="@+id/btn_take_photo"
Copy link
Collaborator

Choose a reason for hiding this comment

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

컨벤션 잡았다 요놈!

android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginStart="12dp"
android:text="사진 촬영"
Copy link
Collaborator

Choose a reason for hiding this comment

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

미리 string.xml에 빼두는게 나려나? 지금 작업은 하는데 이것도 우리 string.xml에 어떻게 뺄지 컨벤션 간단히 컨벤션 정해야할거 같은데

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

나중에 병합해서 한 사람이 작업하는게 좋을 것 같은뎅 다같이 이야기 해보장 !

import dagger.hilt.android.AndroidEntryPoint
import java.util.Calendar

@AndroidEntryPoint
Copy link
Collaborator

@BENDENG1 BENDENG1 Jul 10, 2024

Choose a reason for hiding this comment

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

@androidentrypoint 어노테이션을 ImageUploadDialog 이미지 업로드 다이얼로그도 동일하게 적용시켜줘야 할거 같은데?!

@minju1459 minju1459 requested review from BENDENG1 and SsongSik July 10, 2024 12:08
@minju1459 minju1459 merged commit 65ef099 into main Jul 10, 2024
@minju1459 minju1459 changed the title [BSVR-82/FEAT] 직관 등록 1차 UI 구현 [BSVR-82/FEAT] 시야 후기 등록 1차 UI 구현 Aug 7, 2024
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.

[FEATURE] 직관 등록 1차 UI 구현
3 participants