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

경북대Android_남지연_1주차_과제 #28

Open
wants to merge 28 commits into
base: njiyeon
Choose a base branch
from

Conversation

nJiyeon
Copy link

@nJiyeon nJiyeon commented Jun 28, 2024

현재 깃에서 푸시 오류가 계속 나서 njiyeon 브랜치를 새로 파서 올립니다.

또한 연락처 화면의 아이콘 업로드에서 계속 오류가 발생하여 ic_main.xml만을 업로드하였습니다.

이번 과제를 통해 다양한 View 속성에 대해 알아갈 수 있었습니다. 아직 부족한 것이 많다고 느낀 과제였고, 특히나 git 사용에 있어서 미숙한 점이 많다는 것을 느꼈습니다.

[ 아래는 실행 이미지입니다]
image
image
image
image
image
image
image
image

@nJiyeon
Copy link
Author

nJiyeon commented Jun 28, 2024

emoticon mipmap의 ic_main과 ic_main_foreground, ic_main_background 파일에 Vector Asset을 활용하여 넣은 이미지입니다. 깃허브에 업로드가 되지 않아 이렇게라도 사진을 업로드합니다.

Copy link

@mkSpace mkSpace left a comment

Choose a reason for hiding this comment

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

안녕하세요 지연님. 6주 동안 코드 리뷰를 맡게 된 제리 멘토입니다.

작성해주신 코드 잘 읽었습니다. 기능 요구사항에 맞춰서 잘 작성해주신 것 같습니다. 다만 아이콘이 업로드 되지 않아서 실행해보진 못했네요. 다음 리뷰에는 꼭 오류 해결하셨으면 좋겠습니다.

리뷰 남겨드리니 읽어보시기 답변 부탁드립니다 :)


<activity
android:name=".DetailActivity"
android:exported="true" />
Copy link

Choose a reason for hiding this comment

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

exported가 어떤 역할을 하는 속성일까요?

Copy link
Author

Choose a reason for hiding this comment

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

저장된 데이터에 다른 앱이 엑세스 하도록 허용하는 속성으로, 앞으로의 클론 코딩 프로젝트에서 1주차 과제의 연락처 기능을 이용할 것이라 생각하여 true로 설정하였습니다.

또한, 컴파일 오류가 발생하여 찾아보니, API 레벨이 31 이상인 경우 해당 속성을 명시적으로 설정하지 않아 발생한 것으로 보여 추가하였습니다.

Copy link

Choose a reason for hiding this comment

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

모든 액티비티가 그 조건에 부합하나요?

val notes = intent.getStringExtra("notes") ?: ""

if (name.isNotEmpty()) {
findViewById<TextView>(R.id.name_label).text = "이름"
Copy link

Choose a reason for hiding this comment

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

디자인 관련해서 마땅히 있어야 하는 값이라면 xml에 세팅해주세요! 프로그래밍을 통해 값을 동적으로 바꾸는것보다 xml에 static하게 구성하는게 성능 상 이점이 있습니다.

Copy link
Author

Choose a reason for hiding this comment

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

넵 !!

startActivity(detailIntent)
}

contactList.addView(contactView)
Copy link

Choose a reason for hiding this comment

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

CardView를 직접 inflate 해서 LinearLayout에 추가하셨네요. 매번 View를 직접 생성해서 넣어주면 어떤 이슈가 있을까요?
그리고 이를 위한 해결법은 어떤게 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

매번 view를 생성해서 넣으면 앱의 메모리 사용량의 증가를 야기하는 문제점이 있음을 확인하였습니다. 또한 코드 자체가 복잡하고 유지보수가 어렵다는 문제가 생긴다는 것을 알 수 있었습니다. 이러한 문제점은 RecyclerView를 이용해서 해결할 수 있다는 것을 확인하였습니다. 다음 과제에서는 RecyclerView를 활용하도록 하겠습니다 !

Copy link

Choose a reason for hiding this comment

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

좋습니다!

Comment on lines 25 to 35
val etName = findViewById<EditText>(R.id.et_name)
val etPhone = findViewById<EditText>(R.id.et_phone)
val etEmail = findViewById<EditText>(R.id.et_email)
val etBirthdate = findViewById<EditText>(R.id.et_birthdate)
val rbFemale = findViewById<RadioButton>(R.id.rb_female)
val rbMale = findViewById<RadioButton>(R.id.rb_male)
val etNotes = findViewById<EditText>(R.id.et_notes)
val btnMore = findViewById<TextView>(R.id.btn_more)
val moreForm = findViewById<LinearLayout>(R.id.more_form)
val btnCancel = findViewById<TextView>(R.id.btn_cancel)
val btnSave = findViewById<TextView>(R.id.btn_save)
Copy link

Choose a reason for hiding this comment

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

멤버 변수로 관리하지 않고 지역 변수로 선언하신 이유가 있으실까요?


btnMore.setOnClickListener {
moreForm.visibility = LinearLayout.VISIBLE
btnMore.visibility = View.GONE
Copy link

Choose a reason for hiding this comment

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

팁입니다! View.isVisible 확장 함수를 찾아보세요! 좀 더 직관적으로 표현할 수 있습니다.

Copy link
Author

Choose a reason for hiding this comment

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

넵 !

Comment on lines 38 to 41
setupEditTextFocus(etName)
setupEditTextFocus(etPhone)
setupEditTextFocus(etEmail)
setupEditTextFocus(etNotes)
Copy link

Choose a reason for hiding this comment

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

EditText 클릭 시에만 키보드를 표시하기 위해 작성하셨다고 했는데 클릭 외에 경우에도 키보드가 표시된 케이스가 있나요?

Comment on lines 144 to 145
val formattedDate = "${selectedYear}-${String.format("%02d", selectedMonth + 1)}-${String.format("%02d", selectedDay)}"
editText.setText(formattedDate)
Copy link

Choose a reason for hiding this comment

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

Date Format을 위한 SimpleDateFormat 클래스가 존재합니다. 이를 활용해보세요~


<ScrollView
android:layout_width="0dp"
Copy link

Choose a reason for hiding this comment

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

ConstraintLayout에서 match_parent 작성하는 방식을 제대로 작성해주셨네요. 👍

android:layout_width="0dp"
android:layout_height="0dp"
app:layout_constraintTop_toTopOf="parent"
app:layout_constraintBottom_toTopOf="@+id/fab"
Copy link

Choose a reason for hiding this comment

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

이렇게 fab 까지를 높이로 설정하면 fab 까지만 표시가 되겠네요. height도 parent의 높이를 따라가고 fab을 그 위에 얹는건 어떨까요?

android:id="@+id/fab"
android:layout_width="56dp"
android:layout_height="56dp"
android:layout_margin="16dp"
Copy link

Choose a reason for hiding this comment

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

layout_margin은 기본적으로 모든 방향의 margin을 표현합니다. 꼭 필요한 bottom, end에만 주는게 좋아보입니다.

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

Successfully merging this pull request may close these issues.

2 participants