-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feat/#10 홈 화면 구현 #36
The head ref may contain hidden characters: "feat/#10-\uD648-\uD654\uBA74-\uAD6C\uD604"
Feat/#10 홈 화면 구현 #36
Conversation
- network 연결상태를 알 수 있는 checker 구현 - api 통신을 하기위한 di 의 module 정의 진행 - 기본적인 string을 가져다 쓸 수 있도록 정의
app/src/main/AndroidManifest.xml
Outdated
<category android:name="android.intent.category.BROWSABLE" /> | ||
|
||
<data android:host="oauth" | ||
android:scheme="kakao6c28960f69d4c7f00043b02d890dd6e0" /> |
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.
굳이 수정은 안해도 되지만 scheme 같은건 나중에 string으로 모아보면 편할거 같아유~
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으로 모아놓는게 좋을 거 같네요 ㅎㅎ 감사합니다.
) { | ||
enum class AuthProvider { | ||
GOOGLE, KAKAO, APPLE | ||
} |
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.
별도의 enum class로 빼주는게 추후에 다른 곳에서 사용시 편할 수 있슴다.
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 accessToken: String, | ||
@SerializedName("refresh_token") | ||
val refreshToken: String, | ||
) |
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.
Token이라는 단어는 여기서만 사용되면 상관이 없지만 좀 더 범용적으로 사용되는 단어이니
별도의 data class로 빼고 SignUpResponseToken 과 같은 specific한 네이밍을 하면 좋슴다.
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.
오 좋네요! 수정하겠습니다!!
holder.bind(getItem(position)) | ||
} | ||
} | ||
|
||
class DdayAdapterViewHolder(private val binding: ItemDDayBinding, private val clickEvent: (id: Int) -> Unit = {}) : RecyclerView.ViewHolder(binding.root) { |
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.
한 file의 2가지 이상의 class가 있는건 지양하는게 좋습니다.
diffutil은 companion object로 선언해도 충분할듯하고~!
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.
diffutil companion object로 선언했어요~ viewholder 클래스들을 어떻게 분리할지 조금 더 고민해볼께요!!
|
||
override fun onResume() { | ||
super.onResume() | ||
window.statusBarColor = getColor(this, R.color.yellow_fefaea) |
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.
한줄짜리함수도 좀 더 가독을 높이려면
setStatusBarColor() 와 같이 추상화 단계를 쪼개면 좋습니다.
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.
리뷰 반영해서 반복되는 함수 가독성 높이고, 사용하기에 용이하도록 protected
속성으로, BaseActivity
와 BaseFragment
에 추가했습니다! 감사합니다.
|
||
@RequiresApi(Build.VERSION_CODES.O) | ||
private fun checkDate(dateString: String): Boolean { | ||
val formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd") |
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.
"yyyy-MM-dd" 이런부분도 상수화 해주면 좋습니다.
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.
상수화했습니다!
setSelection(text.length) | ||
} | ||
setLabelBirthWarn(text.length == 10) | ||
} |
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.
매직넘버~~
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.
if else문이 많은데 when으로도 수정 될거 같아요
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.
매직넘버 수정했고, when절로 수정했습니다!! 감사합니다!!
class MyPageViewModel @Inject constructor() : BaseViewModel() { | ||
private val _closeButtonClicked: SingleLiveEvent<Unit> = SingleLiveEvent() | ||
val closeButtonClicked: LiveData<Unit> = _closeButtonClicked | ||
private val _editButtonClicked: SingleLiveEvent<Unit> = SingleLiveEvent() |
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.
변수별로 행구분은 해줘요~
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.
행구분 했습니다~ 감사합니다!!
MutableLiveData() | ||
val myColorStatus: LiveData<MutableMap<MyColorType, ColorStatus>> = _myColorStatus | ||
|
||
init { |
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.
init 에 별도의 함수를 넣는것도 좋지만
개인적으론 사용하는 view에서 onResume 이나 onCreate 이후에 fetchXXX() 함수들을 호출하거나
viewModel.initViewModel() 과 같은 것들을 불러주는게 좋다고 생각해요
-> 생명주기에 대해 얽혀서 발생하는 이슈들이 적어질 겁니다.
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.
오 감사합니다! 네트워크 모듈 연결 후 참고해서 수정하겠습니다!!
🧨이슈
🔍 상세 내용 (해결 내용)
💡 참고자료 및 공유할만한 자료 (선택)
❕후속 진행 이슈