-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/#795 게시글 화면과 댓글 화면 ux 개선 #807
The head ref may contain hidden characters: "Feature/#795-\uAC8C\uC2DC\uAE00_\uD654\uBA74\uACFC_\uB313\uAE00_\uD654\uBA74_UX_\uAC1C\uC120"
Changes from 67 commits
104212f
8bb393d
052dff1
502c39e
b637847
54659db
627bd2b
d56af64
32241ed
ac0c8cd
2920edd
3eadc97
f411b13
dff0843
3f57cf4
6b81fca
e482b2a
1e75d94
e6da29a
138b1eb
cd85204
ae2ca30
64a995e
42639c2
4ed2a32
eb642c0
0f3bd5e
1fee21b
a46cae7
6ac6b01
912b746
3d834b1
f2cd97a
72f2f1d
a1b3bc9
884923f
d684971
40cbde7
7d45576
56601cf
acca981
d7de471
685ddd9
89f73da
b5b7af3
dd5bc27
393005f
2a03057
1789c6e
1ce8d93
b8e74b8
a3b27f0
b81969c
4798eaf
ea05164
d3260e4
acce77c
9caf086
5bfd687
9b66d3c
f284c3c
b35d159
579662b
3cf6c4d
0f9f24a
41853fe
a81e667
7026a44
58a6314
2552c59
dafff57
114d953
fb6e300
4c16fb4
2ceec5a
bfd2b0c
31d52e6
e7d6060
93e995b
22bad95
b778644
2c26104
49222f8
49dc225
4641cf9
e2eab6c
80bb03b
a9bdbdb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
package com.emmsale.presentation.base | ||
|
||
import androidx.lifecycle.ViewModel | ||
import androidx.lifecycle.viewModelScope | ||
import com.emmsale.data.common.retrofit.callAdapter.ApiResponse | ||
import com.emmsale.data.common.retrofit.callAdapter.Failure | ||
import com.emmsale.data.common.retrofit.callAdapter.NetworkError | ||
import com.emmsale.data.common.retrofit.callAdapter.Success | ||
import com.emmsale.data.common.retrofit.callAdapter.Unexpected | ||
import kotlinx.coroutines.Job | ||
import kotlinx.coroutines.launch | ||
|
||
abstract class BaseViewModel : ViewModel() { | ||
|
||
protected abstract fun changeToLoadingState() | ||
|
||
protected abstract fun changeToNetworkErrorState() | ||
|
||
protected abstract fun changeToSuccessState() | ||
|
||
protected abstract fun onUnexpected(throwable: Throwable?) | ||
|
||
protected abstract fun onRequestFailByNetworkError() | ||
|
||
abstract fun refresh(): Job | ||
|
||
protected fun <T : Any> requestToNetwork( | ||
getResult: suspend () -> ApiResponse<T>, | ||
onSuccess: ((T) -> Unit)? = null, | ||
onFailure: ((code: Int, message: String?) -> Unit)? = null, | ||
onLoading: (suspend () -> Unit)? = null, | ||
onNetworkError: (() -> Unit)? = null, | ||
): Job = viewModelScope.launch { | ||
val loadingJob = launch { onLoading?.invoke() ?: changeToLoadingState() } | ||
when (val result = getResult()) { | ||
is Failure -> onFailure?.invoke(result.code, result.message) | ||
NetworkError -> { | ||
onNetworkError?.invoke() ?: changeToNetworkErrorState() | ||
return@launch | ||
} | ||
|
||
is Success -> onSuccess?.invoke(result.data) | ||
is Unexpected -> onUnexpected(result.error) | ||
} | ||
loadingJob.cancel() | ||
changeToSuccessState() | ||
} | ||
|
||
protected fun <T : Any> commandAndRefresh( | ||
command: suspend () -> ApiResponse<T>, | ||
onSuccess: ((T) -> Unit)? = null, | ||
onFailure: ((code: Int, message: String?) -> Unit)? = null, | ||
onLoading: (suspend () -> Unit)? = null, | ||
onNetworkError: (() -> Unit)? = null, | ||
): Job = viewModelScope.launch { | ||
val loadingJob = launch { onLoading?.invoke() ?: changeToLoadingState() } | ||
when (val result = command()) { | ||
is Failure -> onFailure?.invoke(result.code, result.message) | ||
NetworkError -> onNetworkError?.invoke() ?: onRequestFailByNetworkError() | ||
is Success -> { | ||
refresh().join() | ||
onSuccess?.invoke(result.data) | ||
} | ||
|
||
is Unexpected -> onUnexpected(result.error) | ||
} | ||
loadingJob.cancel() | ||
changeToSuccessState() | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,15 +3,36 @@ package com.emmsale.presentation.common.bindingadapter | |
import androidx.annotation.ColorInt | ||
import androidx.databinding.BindingAdapter | ||
import androidx.swiperefreshlayout.widget.SwipeRefreshLayout | ||
import kotlinx.coroutines.CoroutineScope | ||
import kotlinx.coroutines.Dispatchers | ||
import kotlinx.coroutines.Job | ||
import kotlinx.coroutines.launch | ||
|
||
@BindingAdapter("onRefresh") | ||
@BindingAdapter("app:onRefresh") | ||
fun SwipeRefreshLayout.setOnRefresh(onRefresh: () -> Unit) { | ||
setOnRefreshListener { | ||
onRefresh() | ||
isRefreshing = false | ||
} | ||
} | ||
|
||
@BindingAdapter("app:onRefresh1") | ||
fun SwipeRefreshLayout.setOnRefresh1(onRefreshListener: OnRefreshListener) { | ||
setOnRefreshListener { | ||
val job = onRefreshListener.onRefresh() | ||
if (!job.isCancelled) { | ||
CoroutineScope(Dispatchers.Main).launch { | ||
job.join() | ||
isRefreshing = false | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dispatcher를 메인으로 하신 이유가 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. UI를 변경하는 UI 로직이기 때문입니다. |
||
} | ||
} | ||
|
||
fun interface OnRefreshListener { | ||
fun onRefresh(): Job | ||
} | ||
|
||
@BindingAdapter("app:swipeRefreshColor") | ||
fun SwipeRefreshLayout.setSwipeRefreshColor(@ColorInt color: Int) { | ||
setColorSchemeColors(color) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package com.emmsale.presentation.common.extension | ||
|
||
import android.app.Activity | ||
import android.view.inputmethod.InputMethodManager | ||
import androidx.appcompat.app.AppCompatActivity | ||
|
||
fun Activity.hideKeyboard() { | ||
val imm = getSystemService(AppCompatActivity.INPUT_METHOD_SERVICE) as InputMethodManager | ||
imm.hideSoftInputFromWindow(window.decorView.windowToken, 0) | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,18 @@ | ||||||||||||||||||||
package com.emmsale.presentation.common.extension | ||||||||||||||||||||
|
||||||||||||||||||||
import android.content.Context.INPUT_METHOD_SERVICE | ||||||||||||||||||||
import android.view.inputmethod.InputMethodManager | ||||||||||||||||||||
import android.widget.EditText | ||||||||||||||||||||
|
||||||||||||||||||||
private const val DELAY_SHOW_SOFT_INPUT: Long = 200 | ||||||||||||||||||||
|
||||||||||||||||||||
fun EditText.showKeyboard() { | ||||||||||||||||||||
val imm = context.getSystemService(INPUT_METHOD_SERVICE) as InputMethodManager | ||||||||||||||||||||
postDelayed( | ||||||||||||||||||||
tmdgh1592 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 코드를 사용해보시는 것을 추천드립니다.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 와우! |
||||||||||||||||||||
{ | ||||||||||||||||||||
imm.showSoftInput(this, InputMethodManager.SHOW_IMPLICIT) | ||||||||||||||||||||
setSelection(text.length) | ||||||||||||||||||||
}, | ||||||||||||||||||||
DELAY_SHOW_SOFT_INPUT, | ||||||||||||||||||||
) | ||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
package com.emmsale.presentation.common.extension | ||
|
||
import android.content.res.Resources | ||
|
||
val Float.dp: Float | ||
get() = (this * Resources.getSystem().displayMetrics.density) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package com.emmsale.presentation.common.recyclerView | ||
|
||
import android.content.Context | ||
import android.graphics.Canvas | ||
import android.graphics.Paint | ||
import android.graphics.RectF | ||
import androidx.annotation.ColorRes | ||
import androidx.core.content.ContextCompat | ||
import androidx.recyclerview.widget.RecyclerView | ||
import com.emmsale.R | ||
import com.emmsale.presentation.common.extension.dp | ||
|
||
class DividerItemDecoration( | ||
context: Context, | ||
private val dividerHeight: Float = 0.5f.dp, | ||
@ColorRes private val dividerColor: Int = R.color.light_gray, | ||
) : RecyclerView.ItemDecoration() { | ||
private val paint = Paint().apply { | ||
color = ContextCompat.getColor(context, dividerColor) | ||
style = Paint.Style.FILL | ||
} | ||
|
||
override fun onDraw(c: Canvas, parent: RecyclerView, state: RecyclerView.State) { | ||
super.onDraw(c, parent, state) | ||
|
||
val left = parent.paddingStart.toFloat() | ||
val right = parent.width - parent.paddingEnd.toFloat() | ||
for (i in 0 until parent.childCount - 1) { | ||
val child = parent.getChildAt(i) | ||
val params = child.layoutParams as RecyclerView.LayoutParams | ||
|
||
val top = child.bottom.toFloat() + params.bottomMargin | ||
val bottom = top + dividerHeight | ||
|
||
val dividerRect = RectF(left, top, right, bottom) | ||
c.drawRect(dividerRect, paint) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,12 @@ | ||
package com.emmsale.presentation.ui.feedDetail.recyclerView | ||
package com.emmsale.presentation.common.recyclerView | ||
|
||
import android.graphics.Rect | ||
import android.view.View | ||
import androidx.recyclerview.widget.RecyclerView | ||
import com.emmsale.presentation.common.extension.dp | ||
|
||
class FeedDetailImageItemDecoration( | ||
private val divWidth: Int = 10.dp, | ||
class SpaceItemDecoration( | ||
private val width: Int = 0, | ||
private val height: Int = 0, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저번 리뷰에 남겼던 내용인데 아직 반영이 안 되었네요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
) : RecyclerView.ItemDecoration() { | ||
override fun getItemOffsets( | ||
outRect: Rect, | ||
|
@@ -17,6 +17,7 @@ class FeedDetailImageItemDecoration( | |
super.getItemOffsets(outRect, view, parent, state) | ||
|
||
val position = parent.getChildAdapterPosition(view) | ||
if (position > 0) outRect.left = divWidth | ||
if (position > 0) outRect.left = width | ||
if (position > 0) outRect.top = height | ||
tmdgh1592 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
package com.emmsale.presentation.common.views | ||
|
||
import android.content.Context | ||
import android.util.AttributeSet | ||
import android.view.LayoutInflater | ||
import androidx.constraintlayout.widget.ConstraintLayout | ||
import androidx.core.content.res.use | ||
import androidx.databinding.BindingAdapter | ||
import com.emmsale.R | ||
import com.emmsale.databinding.LayoutBasicInputWindowBinding | ||
import com.emmsale.presentation.common.extension.dp | ||
import com.emmsale.presentation.common.views.BasicTextInputWindow.OnSubmitListener | ||
import kotlin.properties.Delegates | ||
|
||
class BasicTextInputWindow @JvmOverloads constructor( | ||
context: Context, | ||
attrs: AttributeSet? = null, | ||
) : ConstraintLayout(context, attrs) { | ||
|
||
private val binding: LayoutBasicInputWindowBinding by lazy { | ||
LayoutBasicInputWindowBinding.inflate(LayoutInflater.from(context), this, false) | ||
} | ||
|
||
var isSubmitEnabled: Boolean by Delegates.observable(false) { _, _, newValue -> | ||
binding.isSubmitEnabled = newValue | ||
} | ||
|
||
var onSubmitListener: OnSubmitListener by Delegates.observable(OnSubmitListener { }) { _, _, newValue -> | ||
binding.onSubmitListener = newValue | ||
} | ||
|
||
init { | ||
applyStyledAttributes(attrs) | ||
setPadding( | ||
BASIC_TEXT_INPUT_WINDOW_HORIZONTAL_PADDING, | ||
BASIC_TEXT_INPUT_WINDOW_VERTICAL_PADDING, | ||
BASIC_TEXT_INPUT_WINDOW_HORIZONTAL_PADDING, | ||
BASIC_TEXT_INPUT_WINDOW_VERTICAL_PADDING, | ||
) | ||
isClickable = true | ||
addView(binding.root) | ||
} | ||
|
||
private fun applyStyledAttributes(attrs: AttributeSet?) { | ||
context.theme.obtainStyledAttributes( | ||
attrs, | ||
R.styleable.BasicTextInputWindow, | ||
0, | ||
0, | ||
).use { | ||
binding.etBasicInput.hint = it.getString(R.styleable.BasicTextInputWindow_hint) | ||
binding.tvSubmitButton.text = | ||
it.getString(R.styleable.BasicTextInputWindow_submitButtonLabel) | ||
} | ||
} | ||
|
||
fun clearText() { | ||
binding.etBasicInput.text.clear() | ||
} | ||
|
||
fun interface OnSubmitListener { | ||
fun onSubmit(text: String) | ||
} | ||
|
||
companion object { | ||
private val BASIC_TEXT_INPUT_WINDOW_HORIZONTAL_PADDING = 17.dp | ||
private val BASIC_TEXT_INPUT_WINDOW_VERTICAL_PADDING = 8.dp | ||
} | ||
} | ||
|
||
@BindingAdapter("app:onSubmit") | ||
fun BasicTextInputWindow.setOnSubmitListenerBA(onSubmitListener: OnSubmitListener) { | ||
this.onSubmitListener = onSubmitListener | ||
} | ||
|
||
@BindingAdapter("app:isSubmitEnabled") | ||
fun BasicTextInputWindow.setIsSubmitEnabled(isSubmitEnabled: Boolean) { | ||
this.isSubmitEnabled = isSubmitEnabled | ||
} |
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.
changeToNetworkErrorState -> 이것을 전역적으로 구현하는 것은 동의하지 않습니다. 만약 전역적으로 구현한다면, 화면이 네트워크 에러 상태로 변하는 것이겠죠? 하지만 스크랩 요청같은 경우에는 네트워크 에러일 때, 화면을 네트워크 에러 상태로 변하게 하는 것이 아닌, 단순하게 인터넷과의 연결이 끊어졌습니다 라는 토스트 메세지를 띄울 것 같아요. 따라서, changeToNetworkErrorState 는 onNetworkError 로 handleResponse의 인자로 들어가야 할 것 같습니다.
changeToLoadingState, changeToLoadingState
changeToSuccessState와 changeToLoadingState 역시 에러상태와 마찬가지로 함수의 인자로 들어가는 것이 맞다고 생각합니다.
onSuccess 안에서 chnageToSuccessState를 구현한다고 생각되고, changeToLoadingState 도 한개의 로딩상태를 바꾸는 것이 아닌, 다른 로딩상태를 바꿔서 다르게 처리 할 수 있다고 생각하고, 확장성에도 더 좋다고 생각됩니다.
이렇게 하는 것은 어떨까요??
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.
스캇의 말을 듣고 보니 모든 뷰모델 함수가 같은 로딩 상태 처리와 네트워크 상태 처리 하는 것은 옳지 않다고 생각하네요. UI마다 다르게 처리해야 할 수 있으니... 그래서 BaseViewModel을 그냥 없애는 게 나을 것 같다는 생각이 드네요.
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.
저도 동의합니다~ 아무래도 상황별로 다르게 처리해야 하는 경우가 많을 것 같아요.
BaseViewModel 자체는 좋다고 생각합니다.
예를 들어, 프로젝트 초기에 제안드렸던 viewModelScope.launch와 같은 긴 코드를 우리 프로젝트에서는
launchOnUi {...}
,launchOnIO {...}
,laucnhOnDefault {...}
와 같은 형태로 사용하도록 만들거나, 정말 공통적인 코드를 찾아서 Base 형태로 만드는 것은 좋다고 생각해요 : )스캇이 제안해주신 내용과도 비슷한 맥락입니다.
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.
위의 의견을 반영하여 다시 한 번 리팩토링 후 푸시하겠습니다.