-
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/#45 깃허브 로그인 구현 #103
The head ref may contain hidden characters: "Feature/#45-\uAE43\uD5C8\uBE0C_\uB85C\uADF8\uC778_\uAD6C\uD604"
Feature/#45 깃허브 로그인 구현 #103
Conversation
- 서버 API 미구현으로 아직 온전한 기능 구현 상태는 아님
|
||
override fun onNewIntent(intent: Intent?) { | ||
super.onNewIntent(intent) | ||
intent?.data?.getQueryParameter(GITHUB_CODE_PARAMETER)?.let(viewModel::saveGithubCode) |
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.
이렇게만 보면 한 눈에 파악하기 어렵긴 하겠군요
별도의 함수로 분리하겠습니다 👌
super.onCreate(savedInstanceState) | ||
binding = ActivityLoginBinding.inflate(layoutInflater).setContentView(this) | ||
binding.viewModel = viewModel | ||
binding.btnLogin.setOnClickListener(this) |
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.
저는 개인적으로 setOnClickListener 안에 클릭 구현 내용이 들어가있는게 가독성이 좋은 것 같습니다. 뒤에가서 또 R.id.btn 을적어줘야 하는것도 코드 중복인 것 같이 느껴지네요
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.
이후에 다른 소셜 로그인이 적용될 경우를 고려해서 View.OnClickListener를 구현하고, onClick() 메서드에서 로그인 로직을 처리하도록 하려고 하였습니다.
하지만 말씀을 들어보니 지금 당장 필요하지는 않은 것 같네요 👍
나중을 생각해서 코드가 길어지는 것보다 일단 지금 당장의 가독성을 챙기겠습니다.
private val tokenRepository: TokenRepository, | ||
) : BaseViewModel(dispatcherProvider) { | ||
private val _loginState: MutableLiveData<LoginUiState> = MutableLiveData() | ||
val loginState: LiveData<LoginUiState> = _loginState |
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.
개인적으로 LoginUiState 에 Loading 을 넣지 않고, 따로 Loading 을 LiveData 으로 관리하고, Xml의 progress Bar 에 visible에 vm.loading 으로 값을 넣어서 관리하는것이 가독성이 좋을 것 같습니다. LoginUiState 에 Loading 까지 있는 것이 개인적으로 어색하게 느껴집니다.
개인적인 의견이니 반영은 안해도 됩니다.
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.
^^
#️⃣연관된 이슈
📝작업 내용
스크린샷 (선택)
예상 소요 시간 및 실제 소요 시간
💬리뷰 요구사항(선택)