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

[UI/#19] Onboarding / 학교 뷰, 학과/학번 선택 뷰 #23

Merged
merged 21 commits into from
Jul 10, 2023

Conversation

minju1459
Copy link
Contributor

⛳️ Work Description

  • 학교 선택 뷰 UI
  • 학과/학번 선택 뷰 UI

📸 Screenshot

device-2023-07-09-044540.webm

📢 To Reviewers

  • 학번 선택 bottomsheetdialog 부분은 BottomSheetAdapter를 구현하며 처리할 예정입니다.
  • 빠진 부분이 있을 수 있어 디테일한 UI 검토는 서버 통신 끝나마자 들어가겠습니다 !!

@minju1459 minju1459 requested a review from a team as a code owner July 8, 2023 19:51
@minju1459 minju1459 self-assigned this Jul 8, 2023
@minju1459 minju1459 added PULL REQUEST 🚀 pull reqeust 날리기 UI 💐 UI 작업 민주 🌳 Minju's Task labels Jul 8, 2023
@minju1459 minju1459 added this to the UI 구현 milestone Jul 8, 2023
Copy link
Member

@Marchbreeze Marchbreeze left a comment

Choose a reason for hiding this comment

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

고생티비~~~~~

import com.yello.databinding.FragmentDialogDepartmentBinding

class SearchDialogDepartmentFragment :
BindingBottomSheetDialog<FragmentDialogDepartmentBinding>(R.layout.fragment_dialog_department) {
Copy link
Member

Choose a reason for hiding this comment

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

오호 바텀시트도 BindingDialog가 있네요 ! 프로필 뷰에서 활용하겠습니다 ㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋아요오~~~~!!

companion object {
@JvmStatic
fun newInstance() = SearchDialogDepartmentFragment()
}
Copy link
Member

Choose a reason for hiding this comment

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

@JvmStatic 활용법 공부해보고 활용할 수 있으면 좋을듯 ! 공부하면 알려줘용

Copy link
Contributor

@kkk5474096 kkk5474096 Jul 10, 2023

Choose a reason for hiding this comment

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

@minju1459 @Marchbreeze 자바에서는 static을 사용해서 정적으로 메소드를 만들 수 있는데
코틀린에서는 static이란 메소드가 없어서 companion object를 사용해서 안에 함수를 사용하게 되는데
사실상 companion object안에 함수들은 static이 아닌 object 자체가 static이라서
함수 자체를 static처럼 사용하고 싶을 때는 @JvmStatic을 붙여주면 됩니다.

onItemsTheSame = { old, new -> old.schoolname == new.schoolname },
onContentsTheSame = { old, new -> old == new },
)
}
Copy link
Member

Choose a reason for hiding this comment

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

diffutil 따로 빼두니깐 훨씬 더 깔끔하네요 ~~

android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_marginStart="8dp"
android:text="@{data.schoolname}"
Copy link
Member

Choose a reason for hiding this comment

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

활용 좋아요 ~~~~~

android:textSize="16sp"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent" />
Copy link
Member

Choose a reason for hiding this comment

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

이거 textAppearence로 사전 설정되어 있는 속성으로 글꼴, 크기 등등 한번에 지정해주시는게 좋아요 ~~
android:textAppearance="?attr/textAppearanceHeadline3"
이렇게요 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

감사합니당!! 빠르게 수정하도록 하겠습니다 !!

Copy link
Member

@b1urrrr b1urrrr left a comment

Choose a reason for hiding this comment

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

온보딩 구현하시느라 고생하셨습니다!!

initSearchDeaprtmentBtnClickListener()
}

private fun initSearchIDBtnClickListener() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private fun initSearchIDBtnClickListener() {
private fun initSearchIdBtnClickListener() {

Fragment 형식이랑 통일하는 건 어떤가요?

Comment on lines 44 to 46
android:layout_marginStart="16dp"
android:layout_marginTop="12dp"
android:layout_marginEnd="16dp"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
android:layout_marginStart="16dp"
android:layout_marginTop="12dp"
android:layout_marginEnd="16dp"
android:layout_marginHorizontal="16dp"
android:layout_marginTop="12dp"

이렇게 작성할 수도 있어요!

override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
super.onViewCreated(view, savedInstanceState)
initSearchIDBtnClickListener()
initSearchDeaprtmentBtnClickListener()
Copy link
Member

Choose a reason for hiding this comment

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

오타 발견ㅋㅋ
참고로 맥북 기준 ⇧F6으로 한번에 네이밍을 수정할 수 있습니다!


class SchoolAdpapter : ListAdapter<MySchool, SchoolAdpapter.SchoolViewHolder>(diffUtil) {
override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): SchoolViewHolder {
Timber.d("onCreateViewHolder")
Copy link
Member

Choose a reason for hiding this comment

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

테스트 완료하셨다면 불필요한 Timber도 제거하는 것이 가독성에 좋을 것 같아요!

Comment on lines 31 to 33
fun setlist(list: MySchool) {
Timber.d("set list : $list")
binding.data = list
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fun setlist(list: MySchool) {
Timber.d("set list : $list")
binding.data = list
fun setlist(school: MySchool) {
binding.data = school

학교 하나에 대한 변수가 인자로 전달되는데 list라는 네이밍은 조금 부적합한 것 같습니다!

Comment on lines 32 to 33
android:layout_width="24dp"
android:layout_height="24dp"
Copy link
Member

Choose a reason for hiding this comment

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

높이나 너비에 고정 dp값을 지정하는 것은 지양해주세요!
다른 컴포넌트들도 한번씩 확인 부탁드립니다!

Comment on lines 9 to 14
android:id="@+id/layout_school_dialog"
android:layout_width="match_parent"
android:layout_height="match_parent">

<TextView
android:id="@+id/tv_dialog_school"
Copy link
Member

Choose a reason for hiding this comment

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

where에 대한 단어가 컴포넌트마다 다른 것 같은데 통일시켜주는 것이 좋을 것 같습니다!

Comment on lines 28 to 30
android:layout_marginStart="16dp"
android:layout_marginTop="26dp"
android:layout_marginEnd="182dp"
Copy link
Member

Choose a reason for hiding this comment

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

큰 값을 margin이나 padding으로 지정하게 되면 화면 호환성에 문제가 생길 수 있습니다! 요 컴포넌트에서는 marginEnd를 지정하지 않는 것이 적합해보입니당

Copy link
Member

@b1urrrr b1urrrr left a comment

Choose a reason for hiding this comment

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

뷰 진짜 잘 짰어요 너무 고생하셨습니다🥺
LGTM!!!!!!!!!

@minju1459
Copy link
Contributor Author

뷰 진짜 잘 짰어요 너무 고생하셨습니다🥺
LGTM!!!!!!!!!

리드님 최고......💛💛

@minju1459 minju1459 merged commit 5340649 into develop Jul 10, 2023
1 check passed
@b1urrrr b1urrrr deleted the origin/#19-onboarding-school_studentid branch July 18, 2023 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PULL REQUEST 🚀 pull reqeust 날리기 UI 💐 UI 작업 민주 🌳 Minju's Task
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[UI] OnBoarding / 학교 선택 뷰, 학과 학번 선택 뷰
4 participants