-
Notifications
You must be signed in to change notification settings - Fork 75
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
base: team-01
Are you sure you want to change the base?
Conversation
* refact: 피드백 적용 - 카멜 케이스로 변수명 변경 * chore: 피드백 적용 - History 대신 Histories로 메소드 명 변경 메소드 이름으로 명확하게 어떤 기능을 하는 것인지 알 수 있도록 변경하였음 * chore: 피드백 적용 - styles.xml 의 item 한 줄로 변경 일반적인 컨벤션을 따르도록 변경
[Android] GET /api/histories 의 json 데이터 구조 변경
* refact: 네트워크 관련 코드 분리 History뿐 아니라 Todo에서도 네트워크를 사용해야 함으로 공통으로 쓸 수 있도록 분리 * refact: HistoryRepository를 공용으로 사용할 수 있도록 위치 및 파일명 변경
[Android] TO-DO Repository 구현
* feat: Todo 커스텀 뷰 구현 동일한 뷰가 중복되어 사용된다고 판단해 커스텀 뷰로 구현 진행 * feat: Todo 커스텀 뷰 구현 동일한 뷰가 중복되어 사용된다고 판단해 커스텀 뷰로 구현 진행 * feat: todo_item 레이아웃 생성 리사이클러뷰 적용 * chore: 클래스명 및 위치 변경 * 아이템리스트 swipe 구현중 Co-authored-by: DESKTOP-UIQTHKE\User <[email protected]>
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
패키지명이 todo_list 인가요? 특별한 이유가 없다면 소문자로 작성되는것이 컨벤션이라 맞추는게 좋을거 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
변경 완료했습니다. 감사합니다!
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요런 기능은 별도의 함수로 묶으면 보기에 조금 더 편해서 나중에 좋은 점이 있습니다. 혹은 task 가 바뀌게 되더라도 별도의 함수로 묶어 놓으면 바꿔야 할 부분이 명확하게 보일거 같습니다. 분리된 메소드의 위치는 activity/viewmodel 두 군데가 있을텐데 어느 부분이 더 일관적일지 보셔서 추가하시면 좋을거같네요~
There was a problem hiding this comment.
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 = 방향 >> 왼쪽으로 스와이프했을때 작동 |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 클래스가 왜 필요했는지에 대한 주석이 있어도 좋습니다. 리사이클러뷰를 그냥 사용하지 않고 추가 기능을 넣은 이유라던가, 이게 없으면 무엇이 동작하지 않는다던가 등의 설명정도면 어떨까요? 혹은, 클래스 이름 자체를 어떤 역할을 하는것인지 구체적으로 적어봐도 좋겠네요.
@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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
각 값들이 String이 주입되는 형태인데, BindingAdapter을 새로 사용하신 이유가 있으실까요?
(android:text) 를 사용하지 않고)
private lateinit var tvTitle: TextView | ||
private lateinit var tvBadgeCount: TextView | ||
private lateinit var btnAddTask: ImageButton | ||
private lateinit var recyclerViewTodo: RecyclerView | ||
private val todosAdapter = TodoAdapter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존 코드처럼 뷰 변수를 따로 선언하고 커스텀뷰 구성을 할 수도 있으나, 요즘은 ViewBinding 을 이용하여 이런 코드들을 간단하게 만드는 편입니다. 즉 Activity/Fragment 등이 아닌 커스텀 뷰 에서도 바인딩을 사용 할 수 있습니다.
사용하는게 어렵지 않고 꽤 많은 코드를 줄일 수 있는데, 학습이라 생각하고 ViewBinding 을 사용하도록 구현해보는것을 추천드립니다.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이쪽 변수 이름도 되도록이면 컨벤션에 맞게 부탁드립니다!
<LinearLayout | ||
android:layout_width="match_parent" | ||
android:layout_height="108dp" | ||
android:background="@color/white" | ||
app:layout_constraintRight_toRightOf="parent" | ||
app:layout_constraintTop_toTopOf="parent" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 높이만을 위한 뷰인가요? 그렇다면 frameLayout 을 고려해보는게 어떨까 싶습니다. 혹은, 전체 depth 를 하나로 유지하면서 별도 뷰 공간 없이 할 방법을 찾으면 더 좋겠습니다. 이런 뷰 요소는 나중에 오해를 받을수도 있어요! (어떤 역할인지 한번 물억보게 됨0
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() | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
커스텀 뷰 요소를 만드는 기본적인 방법입니다!
추가로, 위에 말씀드린 뷰바인딩/데이터 바인딩을 이용한 방법으로 구현하면 이런 필드 또한 별도 Styleable 로 선언하지 않고 사용 할 수도 있습니다. binding 레이아웃에서 사용하는 뷰는 해당 커스텀 뷰의 필드를 참조하여 xml 에서 설정 가능한 어트리뷰트를 자동으로 만들어줍니다.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트를 시도해보셨군요 👍
시작이 반이니, 추가되었으면 하는 부분을 적어보겠습니다.
간단한거부터 보면, 현재 테스트들은 전부 assert 가 없습니다. 즉, 결과를 확인하지 않습니다.
특정 메소드가 성공했는지 실패했는지 확인 할 수 있는 코드가 들어가야만 테스트 코드로써의 의미가 추가됩니다.
요건은 급하지 않으니 프로젝트 구현하면서 조금씩 다듬어 나가면 좋겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertThat을 이용해 테스트케이스를 수정했습니다.
감사합니다~
질문 사항 답면
연습하거나, 다른사람이 쓴 코드를 보거나, 공식 문서를 자세히 보거나 했던거 같아요. 제 경우는 일단 돌려보고 구현 스펙상 자세히 알아야 하는 부분이 생기면 그 부분부터 역할을 알아보려고 했었습니다. 또한, RecyclerView 같은게 왜 꼭 필요했을지, 정말 효율적인 방법은 맞는지 고민해보면 생각이 더 넓어지더라구요!
현업에서의 경험을 말씀드리면, 해상도 대응이 많을수록 -> 드는 리소스느 더 필요하다 입니다. 뷰는 여러개로 늘어날 수 있으나, 공통되는 로직을 어떻게 반복하지 않아도 다양한 뷰 형태에서 사용 가능할지 포인트를 잡고 구현해보면 좋겠습니다. Clean architecture 을 조금 공부해봐도 좋을거 같아요. |
Espresso를 이용해 TestCase 작성 예정
assertThat 적용
안녕하세요 ! Android 1팀 Jung Park과 wooki 입니다.
아직 완벽하게 구현되지 않아서 부족한 점이 많습니다 ㅠㅠ
우선 구현된 부분까지 정리해서 PR 요청 드립니다.
미진행 사항
📝 Description
Issue: #47 #48
💽 Commits
구현 사항 요약
Todo의 리사이클러 뷰 구현
커스텀 뷰 구현
Todo Repository 구현
🖼 결과
🤷♂️ 질문사항
감사합니다 !