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

DEP-157 feat: dialog, taost util 제작 #45

Merged
merged 3 commits into from
Nov 25, 2022
Merged

Conversation

kimhyeing
Copy link
Contributor

💁‍♂️ 변경 내용

AS-IS

  • dialog util이 없었습니다
  • toast가 UT 기준 디자인이었습니다

TO-BE

  • dialog를 손쉽게 띄울 수 있습니다
  • toast의 디자인이 변경되고, 띄우는 시간을 (duration)을 설정할 수 있게 수정했습니다

📢 전달사항

dialog는 flag, 확인 버튼 눌렀을 때의 action을 전달하도록 했고
toast는 text를 전달하도록 했습니다.
dialog에서 title, description, emoji 등의 정보를 다 전달하지 않도록 한 이유는 정보양이 많은 것 같아서였는데,,, dialog가 계속 늘어난다면 데이터를 전달하는 방식으로 바꾸는게 좋은가? 고민이네요 (만약 같은 dialog를 여러 시점에서 호출한다면 지금의 방식이 더 편할 것 같긴합니다)

  • DELETE_HABIT_NO_HISTORY
    image

  • DELETE_HABIT_WITH_HISTORY
    image

  • DELETE_HABIT_WITH_MATE
    image

  • CANCEL_HABIT_CREATE
    image

  • CANCEL_HABIT_MODIFY
    image

  • 다이얼로그 사용법

ThreeDaysDialogFragment.newInstance(
            dialogMode = 위의 값들 중 하나,
            onPositiveAction = 우측버튼 눌렀을 때 벌어질 행동에 대한 함수
        ).show(프래그먼트매니저(supportFragmentManager, childFragmentManager,..), ThreeDaysDialogFragment.TAG)
  • 토스트 사용법
ThreeDaysToast().show(context, 표시할 문구)

! 스낵바는 아직 한 곳 밖에 안쓰여서 일단 안 만들긴했는데,, 차차 제작해보겠습니닷
(스낵바를 아직 써본적이 없어서 얼마나 걸릴질? 모르겠어서 우선 완성한데까지라도 머지하자! 라는 심정에서 먼저 올립니닷)

@kimhyeing kimhyeing self-assigned this Nov 23, 2022
@kimhyeing kimhyeing requested a review from a team as a code owner November 23, 2022 16:16
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@juhwankim-dev juhwankim-dev left a comment

Choose a reason for hiding this comment

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

확인했습니다.
다이얼로그 사용법까지 작성해주시고.. 최고!
고생하셨습니다!

@kimhyeing kimhyeing merged commit 90fd400 into develop Nov 25, 2022
@kimhyeing kimhyeing deleted the feature/DEP-157 branch November 25, 2022 08:50
@@ -8,11 +8,11 @@ plugins {
}

android {
compileSdk 32
compileSdk 33
Copy link
Member

Choose a reason for hiding this comment

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

SDK 버전이 33으로 올라갔네요-
변경하게된 이유가 있을까요?
그리고 다른 모듈에서도 동일하게 33버전으로 맞추어야하는지도 궁금합니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

plugin version업데이트를 하면서 32로 하면 주의?? 표시가 뜨더라구여(33으로 올리라는)
그래서 현재 다른 모듈도 33으로 올리는걸 다른 브랜치에서 하고있습니닷!

import android.view.View
import androidx.databinding.BindingAdapter

class OnSingleClickListener(
Copy link
Member

Choose a reason for hiding this comment

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

interval 동안 이벤트를 무시하는 동작으로 보아 이 클래스는 쓰로틀링을 구현하신 것으로 이해했는데 맞을까요?

맞다면 이름이 좀더 명확하게 기능을 드러내면 좋을 것 같습니다.
Single Click 은 Double Click 등을 떠올리게 할 수도 있어보여서요.

추가로 ThreadSafe 하게 구현할 필요는 없는지도 궁금합니다.
현재 구현은 여러 스레드가 동시에 접근하는 경우, 의도한대로 동작하지 않을수도 있어보여서요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

대체로 코틀린에선 서버에 이중으로 데이터를 보내지 않기하기 위해? 하는 클릭 이벤트 이름을 onSingleClickListner라고 지어서 그렇게 지었습니다
single click은 오직 한번의 클릭을 유도한다라는 의미를 내포하기 위해 지었는데 헷갈리신다면 다른 이름을 찾아보겠습니닷
ThreadSafe...는 생각을 안해봤는데 아직 스레드에 대한 개념이 적어서 이건 한 번 찾아보겠습니당 ㅠㅠ

import kotlin.math.roundToInt

val Int.dp: Int
@JvmSynthetic inline get() = TypedValue.applyDimension(
Copy link
Member

Choose a reason for hiding this comment

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

@JvmSynthetic 은 처음봐서 잘 몰라서 드리는 질문인데요.
언제 쓰는 애너테이션지 혹시 알려주실수있나요?
문서를 참고해도 무슨내용인지 잘 와닿지 않아서요 ㅠ.ㅠ
https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.jvm/-jvm-synthetic/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앗 저도 이건 예전 코드에있던걸 가져온거라 저도 JvmSyntehtic에 대해 알아봤는데 잘 모르겠더라구요..ㅠㅠ 이건 에전에 하셨던 분께 한번 여쭤보겠습니다!

this.isVisible = isVisible
}

fun View.gone() {
Copy link
Member

Choose a reason for hiding this comment

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

안드 뉴비를 위해 visible, invisible, gone 이 어떻게 다른건지 설명 부탁드려도될까요?
단어로보면 visible - invisible 이 대응되는 동작인거같은데,
코드에서 보면 visible - gone이 대응되고, invisible 은 별개의 동작으로 보여서요.

Copy link
Member

Choose a reason for hiding this comment

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

goneinivisible은 둘 다 화면에서 보이지 않게 한다는 점은 같지만
gone은 공간을 차지하지 않게 치워버리는 느낌이고
invisible은 공간은 차지하지만 보이지만 않게 하는 느낌입니다!

this.isVisible = false
}

fun View.goneIfNeeded() {
Copy link
Member

Choose a reason for hiding this comment

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

gonegoneIfNeeded 모두 동일한 결과가 나올거같은데요.
구분해서 구현하신 이유가 궁금합니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좀 더 상황에 대해서 편하게 쓰기위해..? 사용했습니다
만약 visible을 gone으로 바꿔야하는 상황이 올 때는
View.goneIfNeeded에 있는 상황을 써야하는 상황이 오기 때문에 작성을 했습니당 (대체로 그렇게 바뀌는 경우가 많아서요)
gone에 대한 작성은 이러한 경우 없이 gone을 만들게될 일반적인 상황에 대응하기 위해 작성했습니다
(헷갈리시다면 goneIfNeeded는 삭제할게요!)

companion object {
const val TAG = "ThreeDaysDialogFragment"

const val NOT_INIT = -1
Copy link
Member

Choose a reason for hiding this comment

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

Dialog 구분할 때, 상수보다는 enum 을 사용하는 것도 좋을 것 같습니다.
가능하다면 int 보다는 타입을 정의해서 사용하는게 좀더 컴파일러에게 정보를 많이 제공할 수 있고, 실수할 수 있는 가능성을 줄일 수 있을 것 같아요.

현재는 작성자가 의도하지 않은 5 같은 값을 입력해도 컴파일하는데 문제가 없고, runtime 에 IllegalStateException 등이 발생함으로써 인지할 수 있는데요.
enum 으로 정의한다면 잘못된 값을 넣으려고 할 때 컴파일타임에 에러로 발견할 수 있으니까요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 알겠습니다!!!

companion object {
const val TAG = "ThreeDaysDialogFragment"

const val NOT_INIT = -1
Copy link
Member

Choose a reason for hiding this comment

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

dialog 를 구분할 때, 사용처의 컨텍스트를 담기보다는 레이아웃 구성에 따라 구분하는 방법도 가능할거같아요.
현재의 구분방법을 따른다면, 화면이 늘어날때마다 다이얼로그 구분하는 코드도 계속 수정되어야하는데요. 레이아웃 기준으로 구분한다면 내용만 바꿔가면서 템플릿은 재활용 가능할 것 같아요. 약간 Solid 의 OCP(개방폐쇄원칙) 느낌..?

  • 이모지, 제목, 액션버튼, 취소버튼
  • 이모지, 제목, 설명, 액션버튼, 취소버튼
  • 제목, 설명, 액션버튼, 취소버튼

근데 쓰다보니 동적으로 마진 제어하는 부분이 이건가싶네요.. 같은 컴포넌트라도 여백이 다를 수 있겠네요 ㅠㅠ
무튼 의견 부탁드립니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 이게 저도 고민을 참 많이 했는데,,, 역시 파라미터로 가는 방법이 좋겠죠..? 한 다이얼로그를 여러곳에 쓴다면 매번 파라미터를 넘겨주는 과정이 귀찮을 것 같아서 상수로 넘기는 방법을 택했는데
데이터를 파라미터로 넘기는 방법으로 바꿔보겠습니다!!
(마진 제어는 이모지가 있나/없나 , 설명이 있나/없나 로 구분지어서 저 파일에서 설정할 수 있을 것 같아요)
(아니면 마진값도 넘긴다던가..?)

app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@+id/tv_emoji"
tools:text="작성중인 내용이 있어요\n나가시겠어요?" />
Copy link
Member

Choose a reason for hiding this comment

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

string.xml 에서 값을 관리하고있는데, 직접 값을 넣어주신 이유가 있을까요?
코드에서 변경하지 않았을 때의 기본값으로 이해하면 될까요?
텍스트가 중복되는거같아서 string.xml 값을 사용할 수 있으면 변수로 주입해도 좋을거같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tools:text는 직접 보이는게 아니라 에디터에서만 보이는(안드 스튜디오에서만) 텍스트값입니다! (android:text에 저런식으로 넣었으면 string을 추출하라고 주의가 떴을테지만 tools는 상관이 없습니다!)
제가 그냥 디자인할때 그 크기나 전체적인 디자인을 보려고 임의로 넣은 값입니당

"roomKtx" : "androidx.room:room-ktx:${versions.room}",
"roomRuntime" : "androidx.room:room-runtime:${versions.room}",
"roomRxjava2" : "androidx.room:room-rxjava2:${versions.room}"
"room" : [
Copy link
Member

Choose a reason for hiding this comment

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

혹시 이부분 어떻게 정렬하셨는지 알려주실수있나요? ㅋㅋㅋ
저는 손으로 스페이스 한칸한칸 입력해서 정렬하기 애매해서 자동정렬 되는대로 썼었습니다 ㅠ.ㅠ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저도 사실 이 부분을 몰라서 ^^,.. 노가다로 넣긴햇습니다 탭탭..스페이스...탭...

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.

3 participants