-
Notifications
You must be signed in to change notification settings - Fork 1
[Campus][Feature] 분실물 게시글 수정 기능 구현 #1241
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
base: feature/lostandfound-base
Are you sure you want to change the base?
[Campus][Feature] 분실물 게시글 수정 기능 구현 #1241
Conversation
…o feature/#1214-lostandfound-edit
…b/KOIN_ANDROID into feature/#1214-lostandfound-edit
…om/BCSDLab/KOIN_ANDROID into feature/#1214-lostandfound-modify
kongwoojin
left a comment
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.
고생하셨습니다.
코멘트 확인해주세요
| onModifyArticleClick = { | ||
| navigateToModify(uiState.id) |
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 fileType = | ||
| context.contentResolver.getType(uri) | ||
| ?: "image/${fileName.split(".").last()}" |
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.
fileType.startsWith("image/") 으로 판단해서 image 인 경우만 하도록 하겠습니다.
type 이 null 인 경우는 그냥 무시하는게 나으려나요?
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.
nullable 로 바꾸었습니다. null 일 경우 image 로 판단하지 않고 false 로 판단하여 등록되지 않습니다.
등록되지 않으면 toast message 를 올려줍니다.
| ) : ViewModel(), | ||
| ContainerHost<LostAndFoundModifyState, LostAndFoundModifySideEffect> { | ||
| override val container = | ||
| container<LostAndFoundModifyState, LostAndFoundModifySideEffect>( | ||
| LostAndFoundModifyState(), | ||
| savedStateHandle | ||
| ) { | ||
| val articleId = savedStateHandle.get<Int>(ARTICLE_ID) | ||
| checkNotNull(articleId) | ||
| fetchLostAndFoundDetail(articleId) | ||
| } | ||
| private var originImages: List<ArticleLostAndFound.ArticleLostAndFoundImage> = emptyList() |
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.
container 말씀하시는거죠? 일단 container 고치도록 하겠습니다.
…m/BCSDLab/KOIN_ANDROID into feature/#1214-lostandfound-chore
…m/BCSDLab/KOIN_ANDROID into feature/#1214-lostandfound-logging
…m/BCSDLab/KOIN_ANDROID into feature/#1214-lostandfound-chore
…m/BCSDLab/KOIN_ANDROID into feature/#1214-fetch-lostandfound-v2
…m/BCSDLab/KOIN_ANDROID into feature/#1214-lostandfound-logging
…/BCSDLab/KOIN_ANDROID into feature/#1214-fetch-lostandfound-v2
…/BCSDLab/KOIN_ANDROID into feature/#1214-lostandfound-logging
…com/BCSDLab/KOIN_ANDROID into feature/#1214-lostandfound-logging
[Campus][Feature] 분실물 로깅
…d-v2 [Campus][Feature] 분실물 단일 조회 V2 적용
[Campus][Feature] 분실물 deep link 추가 + 구조 개선
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.
수고 많으셨습니다!
코멘트 확인 한번만 부탁드립니다!
| isScrolling = isScrolling, | ||
| refreshDatePicker = uiState.refreshDatePicker, | ||
| onAddImageClick = { uri -> | ||
| val cursor = context.contentResolver.query(uri, null, null, null, null) |
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.
cursor 사용하는 부분은 viewmodel로 이동하는 건 어떤가요?
이미지 업로드 로직으로 보이는데 이건 비즈니스 로직이라 분리하는 것이 더 좋아 보입니다!
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.
context 객체를 포함하고 있어서 viewmodel에서 사용하긴 부적절해보입니다.
| EditArticleItemDetail( | ||
| type = lostOrFoundType, | ||
| moreDescription = articleData.content ?: "", | ||
| onMoreDescriptionChange = { onUpdateDescription(it) }, |
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.
이 부분에서 리컴포지션마다 함수 객체가 새로 생성되면서 unstable하게되지 않나요?
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.
수정되었습니다~
JaeYoung290
left a comment
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.
수고 많으셨습니다! 다른 의견 없습니다~
This reverts commit d9119a0.
PR 개요
이슈 번호: #1214
PR 체크리스트
작업사항
작업사항의 상세한 설명
분실물 게시글 수정 api 에 잘못된 요청을 수정했습니다.
분실물 작성 에서의 ui component 를 공용 component 로 이동했습니다.
분실물 수정 기능을 추가했습니다.
논의 사항
스크린샷
추가내용