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

[Team-01][Android] Todo 화면 및 레포지토리 구현 #151

Open
wants to merge 11 commits into
base: team-01
Choose a base branch
from

Conversation

banjjak2
Copy link

안녕하세요 ! Android 1팀 Jung Park과 wooki 입니다.
아직 완벽하게 구현되지 않아서 부족한 점이 많습니다 ㅠㅠ
우선 구현된 부분까지 정리해서 PR 요청 드립니다.

미진행 사항

  • 리사이클러 뷰 Swipe 시 버튼 표시
    • Item 크기에 맞게 삭제 버튼의 높이가 동일하게 늘어나야되는데 그렇지 않은 문제 해결해야 함
  • 커스텀 뷰에서 리사이클러 뷰로 데이터 전송 기능
  • Todo 관련 ViewModel 구현

📝 Description

Issue: #47 #48

  • Todo의 리사이클러 뷰 구현
  • 리사이클러 뷰 및 타이틀, 뱃지 등이 공통적으로 사용됨에 따라 커스텀 뷰로 구현해서 재사용성을 높임

💽 Commits

구현 사항 요약

  • Todo의 리사이클러 뷰 구현

    • Item 레이아웃 작성
    • 어댑터 클래스 구현 및 바인딩어댑터 적용
    • Swipe 시 삭제 버튼 표시
  • 커스텀 뷰 구현

    • 속성 커스텀
    • 텍스트 뷰, 뱃지, +버튼 추가
    • 리사이클러 뷰 추가
  • Todo Repository 구현

    • Retrofit을 이용해 네트워크 통신
    • 데이터 추가/삭제/수정 구현
      • 이동 기능은 수정 기능을 이용하면 됨
    • Mock서버에서 데이터를 가져와 History 업데이트

🖼 결과

image

🤷‍♂️ 질문사항

  • 기능(RecyclerView 사용법, ...)을 처음 접하셨을 때 어떤 방식으로 공부하셨는지 궁금합니다.
  • 해상도별로 대응하는 방법이 어떤게 있을까요? 태블릿에 맞게 구현했는데 이것을 핸드폰 해상도로 맞추게 되면 레이아웃과 MainActivity.kt 도 수정되어야 할 것 같은데 어떻게 해야할 지 잘 모르겠습니다..
    • 예를 들어서 핸드폰에서는 내비게이션을 이용해서 Todo, In Progress, Done 프래그먼트에 접근한다면 태블릿에서는 저희 결과 사진처럼 프래그먼트를 굳이 사용하지 않아도 될 것 같은데, 이렇게 되면 MainActivity의 코드도 해상도별로 나눠서 구현되어야 하나요?

감사합니다 !

banjjak2 and others added 7 commits April 12, 2022 11:30
* refact: 피드백 적용 - 카멜 케이스로 변수명 변경

* chore: 피드백 적용 - History 대신 Histories로 메소드 명 변경

메소드 이름으로 명확하게 어떤 기능을 하는 것인지 알 수 있도록 변경하였음

* chore: 피드백 적용 - styles.xml 의 item 한 줄로 변경

일반적인 컨벤션을 따르도록 변경
[Android] GET /api/histories 의 json 데이터 구조 변경
* refact: 네트워크 관련 코드 분리

History뿐 아니라 Todo에서도 네트워크를 사용해야 함으로 공통으로 쓸 수 있도록 분리

* refact: HistoryRepository를 공용으로 사용할 수 있도록 위치 및 파일명 변경
* feat: Todo 커스텀 뷰 구현

동일한 뷰가 중복되어 사용된다고 판단해 커스텀 뷰로 구현 진행

* feat: Todo 커스텀 뷰 구현

동일한 뷰가 중복되어 사용된다고 판단해 커스텀 뷰로 구현 진행

* feat: todo_item 레이아웃 생성 리사이클러뷰 적용

* chore: 클래스명 및 위치 변경

* 아이템리스트 swipe 구현중

Co-authored-by: DESKTOP-UIQTHKE\User <[email protected]>
@banjjak2 banjjak2 changed the title [Android] Todo 화면 및 레포지토리 구현 [Team-01][Android] Todo 화면 및 레포지토리 구현 Apr 13, 2022
@ku-kim ku-kim requested a review from Zimins April 13, 2022 08:24
@ku-kim ku-kim added the review-android Further information is requested label Apr 13, 2022
Copy link

@Zimins Zimins left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!
Repository, Api, Test 등 여러가지 시도를 하고 계신거 같아서 보기 좋습니다.

@@ -1,4 +1,4 @@
package com.example.todo_list
package com.example.todo_list.data
Copy link

Choose a reason for hiding this comment

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

패키지명이 todo_list 인가요? 특별한 이유가 없다면 소문자로 작성되는것이 컨벤션이라 맞추는게 좋을거 같습니다!

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 48 to 68
val task1 = Task(
1,
"테스트하기1",
"콘텐츠테스트1",
"jung",
"doing",
"2022-04-06T15:30:00.000+09:00",
"2022-04-06T15:30:00.000+09:00"
)

val task2 = Task(
2,
"테스트하기2",
"콘텐츠테스트2",
"park",
"todo",
"2022-04-06T15:30:00.000+09:00",
"2022-04-06T15:30:00.000+09:00"
)

binding.todoTodoView.addTasks(listOf(task1, task2))
Copy link

Choose a reason for hiding this comment

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

요런 기능은 별도의 함수로 묶으면 보기에 조금 더 편해서 나중에 좋은 점이 있습니다. 혹은 task 가 바뀌게 되더라도 별도의 함수로 묶어 놓으면 바꿔야 할 부분이 명확하게 보일거 같습니다. 분리된 메소드의 위치는 activity/viewmodel 두 군데가 있을텐데 어느 부분이 더 일관적일지 보셔서 추가하시면 좋을거같네요~

Copy link
Author

Choose a reason for hiding this comment

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

ViewModel을 앱 전체에서 사용할 수 있도록 파일명 및 위치를 변경했습니다.
ViewModel에서 함수로 묶어 사용하는 것이 더 나을 것 같다고 판단해서 ViewModel에 구현했습니다.
감사합니다!

override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) {
Log.d("TAGonSwiped1", "$direction")
Log.d("TAGonSwiped2", "${viewHolder.adapterPosition}") //<< 몇번째 목록을 조작했는지
// direction = 방향 >> 왼쪽으로 스와이프했을때 작동
Copy link

Choose a reason for hiding this comment

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

direction 에 대한 주석 좋아보입니다! 조금 자세하게 적으면 direction 범위가 무엇일 때 방향이 어딘지 적어볼 수도 있겠네요.

import com.example.todo_list.R
import kotlin.math.min

class ItemTouchHelperCallback(val listener: ItemTouchHelperListener) : ItemTouchHelper.Callback() {
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 6 to 19
@BindingAdapter("setTitle")
fun setTitle(view: TextView, title: String) {
view.text = title
}

@BindingAdapter("setContent")
fun setContent(view: TextView, content: String) {
view.text = content
}

@BindingAdapter("setUser")
fun setUser(view: TextView, user: String) {
view.text = user
}
Copy link

Choose a reason for hiding this comment

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

각 값들이 String이 주입되는 형태인데, BindingAdapter을 새로 사용하신 이유가 있으실까요?
(android:text) 를 사용하지 않고)

Comment on lines 16 to 20
private lateinit var tvTitle: TextView
private lateinit var tvBadgeCount: TextView
private lateinit var btnAddTask: ImageButton
private lateinit var recyclerViewTodo: RecyclerView
private val todosAdapter = TodoAdapter()
Copy link

Choose a reason for hiding this comment

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

기존 코드처럼 뷰 변수를 따로 선언하고 커스텀뷰 구성을 할 수도 있으나, 요즘은 ViewBinding 을 이용하여 이런 코드들을 간단하게 만드는 편입니다. 즉 Activity/Fragment 등이 아닌 커스텀 뷰 에서도 바인딩을 사용 할 수 있습니다.

사용하는게 어렵지 않고 꽤 많은 코드를 줄일 수 있는데, 학습이라 생각하고 ViewBinding 을 사용하도록 구현해보는것을 추천드립니다.

Copy link
Author

Choose a reason for hiding this comment

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

binding을 이용해 뷰를 참조하도록 수정했습니다.
감사합니다!

package com.example.todo_list.tasks

interface ItemTouchHelperListener {
fun onItemMove(from_position: Int, to_position: Int): Boolean
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 +28 to +33
<LinearLayout
android:layout_width="match_parent"
android:layout_height="108dp"
android:background="@color/white"
app:layout_constraintRight_toRightOf="parent"
app:layout_constraintTop_toTopOf="parent" />
Copy link

Choose a reason for hiding this comment

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

혹시 높이만을 위한 뷰인가요? 그렇다면 frameLayout 을 고려해보는게 어떨까 싶습니다. 혹은, 전체 depth 를 하나로 유지하면서 별도 뷰 공간 없이 할 방법을 찾으면 더 좋겠습니다. 이런 뷰 요소는 나중에 오해를 받을수도 있어요! (어떤 역할인지 한번 물억보게 됨0

Comment on lines 26 to 39
private fun initAttributes(attrs: AttributeSet?) {
context.theme.obtainStyledAttributes(
attrs,
R.styleable.TodoView,
0,
0).apply {
try {
tvTitle.text = getString(R.styleable.TodoView_title)
tvBadgeCount.text = getString(R.styleable.TodoView_badge_count)
} finally {
recycle()
}
}
}
Copy link

Choose a reason for hiding this comment

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

커스텀 뷰 요소를 만드는 기본적인 방법입니다!

추가로, 위에 말씀드린 뷰바인딩/데이터 바인딩을 이용한 방법으로 구현하면 이런 필드 또한 별도 Styleable 로 선언하지 않고 사용 할 수도 있습니다. binding 레이아웃에서 사용하는 뷰는 해당 커스텀 뷰의 필드를 참조하여 xml 에서 설정 가능한 어트리뷰트를 자동으로 만들어줍니다.

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분은 머리로는 이해가 되는데 코드가 안따라줍니다...🥲 좀 더 고민해보고 적용해보도록 하겠습니다!
감사합니다 !

*
* See [testing documentation](http://d.android.com/tools/testing).
*/
class MockApiTest {
Copy link

Choose a reason for hiding this comment

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

테스트를 시도해보셨군요 👍

시작이 반이니, 추가되었으면 하는 부분을 적어보겠습니다.
간단한거부터 보면, 현재 테스트들은 전부 assert 가 없습니다. 즉, 결과를 확인하지 않습니다.

특정 메소드가 성공했는지 실패했는지 확인 할 수 있는 코드가 들어가야만 테스트 코드로써의 의미가 추가됩니다.
요건은 급하지 않으니 프로젝트 구현하면서 조금씩 다듬어 나가면 좋겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

assertThat을 이용해 테스트케이스를 수정했습니다.
감사합니다~

@Zimins
Copy link

Zimins commented Apr 13, 2022

질문 사항 답면

처음 접하는 api

연습하거나, 다른사람이 쓴 코드를 보거나, 공식 문서를 자세히 보거나 했던거 같아요. 제 경우는 일단 돌려보고 구현 스펙상 자세히 알아야 하는 부분이 생기면 그 부분부터 역할을 알아보려고 했었습니다. 또한, RecyclerView 같은게 왜 꼭 필요했을지, 정말 효율적인 방법은 맞는지 고민해보면 생각이 더 넓어지더라구요!

해상도별 대응

현업에서의 경험을 말씀드리면, 해상도 대응이 많을수록 -> 드는 리소스느 더 필요하다 입니다.
constraintlayout 을 최대한 활용해서 별도 파일 없이도 다 구현되면 좋겠지만, 유저 사용성에 더 철저히 맞추려면 레이아웃 파일을 여러개 둘 수 밖에 없습니다. (최적화 해야하므로)

뷰는 여러개로 늘어날 수 있으나, 공통되는 로직을 어떻게 반복하지 않아도 다양한 뷰 형태에서 사용 가능할지 포인트를 잡고 구현해보면 좋겠습니다. Clean architecture 을 조금 공부해봐도 좋을거 같아요.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-android Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants