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

[feat] 신고하기 UI 구현 #14

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

[feat] 신고하기 UI 구현 #14

wants to merge 61 commits into from

Conversation

ikseong00
Copy link
Collaborator

@ikseong00 ikseong00 commented Jan 7, 2025

Related issue 🛠

Work Description 📝

  • 상단바
  • 사진 업로드
  • 축종
  • 품종
  • 성별
  • 털색
  • 위치
  • 특징
  • 설명
  • 사진 업로드
  • 품종
  • 특징

+) Text Appearance -> feature chip : semibold , 12sp , letter spacing - 0.04
+) CalenderView
+) TODO 제거

Screenshot 📸

실행 이미지

미리보기 이미지

실행 영상

report_ui.mp4

Uncompleted Tasks 😅

  • 여러가지 팝업들
  • 이미지 삭제 시 2/5 줄어드는 것

줄어드는 건 viewModel 에서 observe 속성 써서 감지해야할 것 같은데 이건 더 공부해보겠습니다.

To Reviewers 📢

- setAdapter
- background 설정
- 텍스트 뷰 클릭
- 아이템 클릭
- 텍스트 입력
- Focus 가 생길 때
- 신체적 특징
- 외적 특징
- 성격
Copy link
Collaborator

@nasohee nasohee 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
Collaborator

@MinseoSONG MinseoSONG left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 !

Comment on lines +4 to +12
enum class PhysicalFeatureType(
val feature: String
) {
PUPPY("새끼 강아지에요"),
ADULT("다 큰 성견이에요"),
SMALL("소형견이에요"),
MEDIUM("중형견이에요"),
LARGE("대형견이에요")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 인자로 @StringRes val featureResId: Int를 받고
PUPPY(featureResId = R.string.puppy) 이런 식으로 수정해서 string을 추출하는건 어떨까 싶습니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

" " 안에 있는 값을 상수로 빼서 String Pool 에 넣어서 관리해야 할 것 같습니다!
근데 strings.xml , 상수 둘 중에 어느 방법이 나은진 고민되네요
xml 자체에서 가져다 쓰는 값은 strings.xml 이 확실히 맞는데,
코틀린 코드에서 사용하는 문자열 값을 strings.xml 에 넣어서 관리하면 , 코드가 약간 길어지고, strings.xml 이 많아질 것 같다는 생각이 들긴 합니다.. !

Copy link
Collaborator

Choose a reason for hiding this comment

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

어.. 질문이 잘 이해가 안가는데 코틀린 코드이기 때문에 string을 추출없이 그대로 사용해야겠다 .. ? 라는 생각이 복잡도가 높아지지않을까?라는 걱정 때문일까요??

클린 코드 관점에서 보면 오히려 string을 추출해서 사용하는 것이 더 체계적이고, 장기적으로 유지보수와 확장성을 확보할 수 있습니다.
하드코딩을 방지할 수 있음은 물론이고, 코드 재사용에 있어서, 그리고 마지막으로는 strings.xml에는 UI에 필요한 문자열을 한 곳에서 관리하여, 코드는 로직에 집중하고 UI 관련 리소스는 별도 파일에서 관리함으로써 역할을 분리할 수 있죠 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

하드코딩 대체하는 방법으로 R.strings.xml 대신에 코드에서 사용하는 부분이니 const 로 관리하자는 말이었습니다. 코드적으로 쓰는 문자열도 strings.xml 에서 관리하면 xml + kt 상수들이 합쳐져서 오히려 너무 많아지지 않을까 ? 라는 생각이 들어서 말씀드린거였어요 !

Comment on lines +111 to +130
// 클릭하면 드랍다운이 생김
setOnClickListener {
dropDownHeight = requireContext().dpToPx(DROP_DOWN_HEIGHT)
showDropDown()
binding.svMissingReportContainer.verticalScrollToYPosition(SCROLL_OFFSET)
}
// 드랍다운 아이템이 선택되면 소프트 키보드가 사라지고, 하단 레이아웃이 보임
setOnItemClickListener { _, _, _, _ ->
requireContext().hideKeyboard(windowToken)
clearFocus()
}
// text 가 변경되면 드랍다운의 크기를 줄임
addTextChangedListener { text ->
ReportDummys.dummyBreeds
.filter { it.contains(text.toString()) }
.let { matches ->
dropDownHeight = if (matches.size > DROP_DOWN_MAX_COUNT) {
requireContext().dpToPx(DROP_DOWN_HEIGHT)
} else ViewGroup.LayoutParams.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.

개인적으로 이런 코드 관련 설명형 주석 처리를 안좋아해서 .. 코드 관련 설명은 pr에 추가해주시고 주석은 지우는게 어떨까요? 사실 코드를 잘 작성해주셔서 굳이 주석이 없어도 이해가 잘됩니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://tidy-tumble-f7d.notion.site/AutoCompleteTextView-17c5fdb94a61803e81bccf066054e405?pvs=4

주석 제거하구 참고자료/아티클 공유 에 올렸습니다

val list = currentList.toMutableList()
list.remove(uri)
submitList(list)
// notifyItemChanged(0)
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.

테스트 하면서 놓쳤던 것 같아요 지우겠습니다 !

Comment on lines +24 to +32
@SuppressLint("SetTextI18n")
fun bind(count: Int) {
// 사진 개수 카운트
binding.tvReportDefaultCount.text = (count - 1).toString()

binding.root.setOnClickListener {
// TODO : 사진 이미지 업로드 기능 설정
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

하드코딩 문자열을 사용하기 위해 이 어노테이션을 추가하신 것 같은데 그것보다는 string을 추출해서 하드코딩을 방지하는게 좋지 않을까요??
예를들어,
Hello, %d
으로 추출해두고
binding.tvReportDefaultCount.text = context.getString(R.string.greeting_message, count-1)
이런 식으로 사용가능합니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image
image

1~5 의 여러 숫자를 다뤄야 해서 이런 식으로 해도 괜찮을까요 ?

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_marginHorizontal="5dp"
android:checkable="true"
app:textStartPadding="2dp"
android:letterSpacing="-0.4"
Copy link
Collaborator

Choose a reason for hiding this comment

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

letterSpacing을 따로 주신 이유가 있으실까요? 프로젝트 기초세팅을 할 때 typography에 letterSpacing까지 선언해줬던 기억이 있어서 .. 확인 부탁드립니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

이 부분에 그냥 적용했더니 글이 잘려서 디자이너 분께서도 자간을 줄인 것 같아요.
그래서 figma 에 맞게 적용시켰습니다.

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.

담당 분께서 휴가가셔서
image
이렇게 했습니다.

android:background="@drawable/bg_report_item_default"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent"
app:shapeAppearanceOverlay="@style/ShapeAppearance.Material3.Corner.Small" />
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.

image

위 코드가 회색 default ViewHolder 에 쓰이는 부분인데 , 그냥 크기를 맞추려고 공백을 넣었습니다.
근데 rootview 에 setOnClickListener 를 달 예정이어서 공백을 줄이는게 맞겠네요. 수정하겠습니다

app/src/main/res/values/strings.xml Show resolved Hide resolved
<string name="report_character_feature">성격</string>
<string name="report_additional_description">추가 설명 작성하기</string>
<string name="report_option">(옵션)</string>
<string name="report_description_hint">자유롭게 추가 설명을 작성해주세요!\n\n\n</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 \n\n\n은 뭘까요?? 끝에 들어간다면 그냥 string을 사용한 text를 기준으로 margin이나 padding으로 bottom에 값을 주는게 좋아보입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hint 에 \n\n\n 적용
image
hint 그냥 적용
image

Edittext 에서 쓰는 hint 인데 입력 텍스트가 많아질수록 커지게 하려고 height 를 wrap 으로 했더니 , 기본 크기가 작더라구요. 그래서 꼼수 느낌으로 string 에 엔터를 추가해서 디폴트 크기를 넓히려고 했습니다.
Line 수에 따라서 크기를 kt 파일에서 조정하는 방법도 있을 것 같은데, 이 방법이 편해보여서 적용했습니다.
코드쪽에서 조절하는게 나은지 의견 있으시면 말씀해주세요 !

Copy link
Collaborator

Choose a reason for hiding this comment

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

코드쪽에서 조절하는게 나을 것 같습니다 꼼수가 걸리지 않는다면 다행이지만 너무 잘보여서 🥲
그리고 아직 시간이 많으니 이런 부분에 있어서 대충 넘어가기보다는 꼼꼼히 잘 작성해두는게 좋을 것 같아요 !
데모데이 직전 코드들이 더 엉망일거라 ^_ 9 ..

Comment on lines +15 to +16
app:layout_constraintTop_toTopOf="parent"
app:shapeAppearanceOverlay="@style/ShapeAppearance.Material3.Corner.Small" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분 여백을 말한 거였습니다 ㅠ 하지만 이 코리로 다른 수정 사항까지 발견했다면 오히려 좋아 .. 😀

@@ -0,0 +1,33 @@
package com.example.findu.presentation.type.report

// TODO : Back 과 논의 후 특징 enum naming 정리 후 상수화
Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO 역시 주석처리가 아닌 따로 노션에 아카이빙해두거나 TODO함수를 사용하는게 좋지않을까 싶습니다 !
코드적으로 보기 좋지않아요

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

Successfully merging this pull request may close these issues.

[feat] 신고하기 UI 구현
3 participants