-
Notifications
You must be signed in to change notification settings - Fork 1
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
#28 [feat] search UI and api #29
base: develop
Are you sure you want to change the base?
Conversation
android:layout_height="wrap_content" | ||
android:layout_marginHorizontal="16dp" | ||
android:layout_marginTop="24dp" | ||
app:editText_imeOptions="actionSearch" |
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.
actionSearch 가 적용안된것 같습니다. EditText를 누르면 엔터키가 돋보기로 보여야하는데 체크표시로 보입니다.
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.
eda2005 커밋에서 수정하였습니다!
|
||
private val requestPermission = | ||
registerForActivityResult(ActivityResultContracts.RequestMultiplePermissions()) { results -> | ||
for (result in results) { |
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.
82번라인에서 시작되어 for문을 타면 ACEESS_FINE_LOCATION, ACCESS_COARSR_LOCATION 둘중 하나만 있어도 GPS를 받아오는것 같습니다.
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.
ACEESS_FINE_LOCATION은 어차피 ACCESS_COARSR_LOCATION와 같이 허용되고, ACCESS_COARSR_LOCATION만 허용되었을 경우, ACEESS_FINE_LOCATION를 다시 요청하도록 만들어서 둘 중 하나만 있어도 GPS를 받아오도록 만들었습니다!
requireActivity().onBackPressedDispatcher.addCallback(this, onBackPressedCallback) | ||
} | ||
|
||
override fun onCreateView( |
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.
onCreateView에서는 어떤 layout xml파일을 로드할지 결정하여 로드하여 화면에 보여주고 있습니다.
onViewCreate 함수 안은 layout xml파일이 이미 로드가 완료된 상태여서 거기서 binding.~ 관련된 변수들을 사용하는게 더 안전해보입니다.
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.
58e2c5a 커밋에서 수정했습니다!
for (result in results) { | ||
when (result.value) { | ||
true -> { | ||
getGPSLocation() |
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.
155b8f0 커밋에서 수정했습니다!
private val shopLocalDataSource: ShopLocalDataSource | ||
) : ShopRepository { | ||
override suspend fun getTrendings(): TrendingResult? { | ||
val resp = shopRemoteDataSource.getTrendings().body() |
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.
통신 오류가 났을 경우 api 응답 json 형태가 TrendingResult의 형태가 아닐경우에 대한 예외처리가 없습니다.
getSearchHistory
searchShop 함수도 동일하게 예외처리가 없습니다.
viewmodel의 runcatching으로 이동될 수 있지만 서버에서 보내주는 오류메시지가 필요할 수 있을것 같습니다.
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.
api 호출 시 오류나도록 호출해보고 그 응답값을 커버할 수 있도록 코드를 작성하면 좋을 것 같습니다.
} | ||
|
||
@SuppressLint("MissingPermission") | ||
fun getGPSLocation() { |
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.
GPS 해제하는 코드가 혹시 있나요? 없다면 GPS를 사용하지 않은곳으로 이동하면 해제하는 코드가 필요해 보입니다.
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.
따로 callback을 만들어서 호출하는 것이 아니라 필요할 때 일회성으로 호출하는 코드라 따로 GPS를 해제하는 코드가 필요 없는것으로 알고 있었는데 혹시 제가 잘못 알고 있는것인가요?
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.
https://developer.android.com/training/location/retrieve-current.html
마지막으로 저장된 위치를 가져오는것은 정확한 위치를 의미하지 않은것 같습니다.
서울에서 쩝쩝박사 앱으로 검색을 한 후 부산을 가서 GPS를 끄고 쩝쩝박사에서 검색한다면 마지막 위치인 서울 위치를 가젼올것 같습니다.
위치정보를 2~3번 호출받아 정확한 위치를 사용하는게 정확성이 높아보입니다.
https://developer.android.com/training/location/request-updates
|
||
binding.buttonSearchShopListNaverMap.setOnClickListener { | ||
try { | ||
val naverMapUrl = "nmap://map?" + |
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.
UrlUtil 혹은 생각나는 이름으로 Object 파일 만들어서 makeUrl 이란 함수로 필요한 url을 만드는 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.
bb1446e 커밋에서 수정했습니다!
@@ -0,0 +1,5 @@ | |||
package com.jjbaksa.data.model.shop | |||
|
|||
data class TrendingResp( |
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.
BaseResp를 상속하여 구현하면 에러코드도 담을 수 있어 보입니다.
PR 개요
이슈 번호: #28
PR Checklist
작업사항
작업사항의 상세한 설명
2022/12/22
2022/12/26
2022/12/27
2022/12/28
추가내용
앞으로 진행해야 하는 것