Conversation
There was a problem hiding this comment.
1주차 미션 완료하시느라 정말 수고 많으셨습니다! 😃 ConstraintLayout을 활용해서 뷰들을 부모(parent) 양옆에 연결해 중앙 정렬을 잘 맞춰주셨고, 무엇보다 클릭했을 때 다른 텍스트 색상이 원래대로 돌아오는 단일 선택 로직 짜주신 점이 너무 좋았어요!
다만 커밋을 하실때에는 대문자가 아닌 소문자로 커밋태그를 달아주세요! ex) feat
코드 전반적으로 훌륭하게 동작하지만, 더 완성도 높은 안드로이드 프로젝트를 위해 몇 가지 개선하면 좋을 부분들을 리뷰로 남겼습니다. 또한 pr을 올리실때에는 동작 영상 또는 실행화면을 캡쳐해서 내용을 다 채워주시기 바랍니다!
마지막으로 사진같은 요소들을 올리실때에는 마크다운(Markdown)을 활용하면 훨씬 보기 좋게 작성할 수 있습니다!
ex) 파이프 기호(|)로 칸을 나누고 하이픈(---)으로 두 번째 줄을 구분해 주는 방식
나중에 pr제출하실때 사진이 많다면은 표로 정리해서 올려주세요!
There was a problem hiding this comment.
PR 사진을 보니 happy.png, good.png 처럼 이미지 파일명과 포맷을 그대로 사용하신 것 같아요!
안드로이드에서는 해상도가 깨지지 않고 앱 용량을 줄이기 위해 가급적 PNG 대신 SVG 파일을 변환한 Vector 형식(.xml)을 사용하는 것을 권장하고 있습니다!
그리고 파일 이름 앞에 아이콘이면 ic_, 배경이면 bg_, 일반 이미지면 img_ 같은 접두어(prefix)를 붙여주면 나중에 파일이 많아졌을 때 찾기 훨씬 수월해져요! (예: ic_emotion_happy.xml)
| val text1 : TextView = findViewById<TextView>(R.id.text1) | ||
| val text2 : TextView = findViewById<TextView>(R.id.text2) | ||
| val text3 : TextView = findViewById<TextView>(R.id.text3) | ||
| val text4 : TextView = findViewById<TextView>(R.id.text4) | ||
| val text5 : TextView = findViewById<TextView>(R.id.text5) | ||
| val good : ImageView = findViewById<ImageView>(R.id.good) | ||
| val happy : ImageView = findViewById<ImageView>(R.id.happy) | ||
| val soso : ImageView = findViewById<ImageView>(R.id.soso) | ||
| val bad : ImageView = findViewById<ImageView>(R.id.bad) | ||
| val angry : ImageView = findViewById<ImageView>(R.id.angry) |
There was a problem hiding this comment.
MainActivity를 보면 findViewById가 10번이나 반복되고 있습니다. 요즘 안드로이드 개발에서는 뷰 바인딩(View Binding)을 도입해서 이런 보일러플레이트 코드를 줄이고 Null 안정성을 챙기는 추세입니다! 다음 미션 때 꼭 한번 적용해 보시는 걸 추천해요!
| ViewCompat.setOnApplyWindowInsetsListener(findViewById(R.id.main)) { v, insets -> | ||
| val systemBars = insets.getInsets(WindowInsetsCompat.Type.systemBars()) | ||
| v.setPadding(systemBars.left, systemBars.top, systemBars.right, systemBars.bottom) | ||
| insets | ||
| } | ||
| } |
There was a problem hiding this comment.
현재 ViewCompat.setOnApplyWindowInsetsListener 코드가 맨 아래에 위치해 있는데요! 이 코드는 상태바나 네비게이션 바 등 시스템 UI 영역의 여백을 잡아주는 기초 공사 역할을 합니다. 보통 onCreate 내부의 코드는 아래와 같은 흐름으로 배치하면 읽기가 훨씬 수월해집니다. 예를 들어서
1단계: 기본 설정 및 화면 연결 (super.onCreate, setContentView)
2단계: 화면 여백 및 시스템 UI 설정 (WindowInsetsListener)
3단계: 뷰 초기화 (ViewBinding 초기화)
4단계: 이벤트 리스너 등록 (setOnClickListener 등)
따라서 WindowInsets 코드를 setContentView 바로 아래쪽으로 올려주시면 구조가 훨씬 깔끔해질 것 같습니다!
| good.setOnClickListener { | ||
| resetTextColors() | ||
| text1.setTextColor(Color.parseColor("#F9DC77")) | ||
| } |
There was a problem hiding this comment.
Color.parseColor("#F9DC77") 같은 값들이 하드코딩되어 있습니다
이런 값들을 colors.xml로 분리해 두면, 나중에 다크모드를 대응하거나 텍스트를 수정할 때 한곳에서 관리할 수 있어서 유지보수하기 훨씬 편해져요! 협업할 때도 마찬가지로 다른 팀원이 지정한 색상을 쉽게 가져다 쓸 수 있어 아주 편리합니다! 따라서 다음 부터는 분리해서 미션 부탁드립니다!
| android:id="@+id/text1" | ||
| android:layout_width="wrap_content" |
There was a problem hiding this comment.
현재 xml의 아이디가 text1, good 처럼 되어 있습니다. 나중에 뷰가 많아지면 text3이 어떤 감정이었는지 헷갈릴 수 있어요!
text1 대신 tv_happy (TextView + 감정), good 대신 iv_good (ImageView + 감정)처럼 어떤 뷰인지, 어떤 역할인지 알 수 있게 접두어를 붙여서 이름을 지어주시면 코드를 읽기 훨씬 편해집니다! 다음 미션부터는 꼭 참고해주세요!
| android:text="더없이 행복한 하루였어요" | ||
| android:textColor="@color/black" |
There was a problem hiding this comment.
이런 값들을 res/values/strings.xml과 colors.xml로 분리해 두면, 나중에 텍스트나 색상을 변경해야 할 때 코드를 일일이 찾아다니며 바꿀 필요 없이 해당 xml 파일 한 곳에서만 싹 수정하면 되기 때문에 훨씬 편리해집니다!
2 participants
📌 PR 제목
🔗 관련 이슈
Closes #이슈번호
✨ 변경 사항
🔍 테스트
📸 스크린샷 (선택)
🚨 추가 이슈