-
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
Feature/tgyuu/#78 #79
Conversation
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.
고생하셨어요 태구 상상상상상상의 나라 에버랜드 ~
eventContent: String, | ||
eventLocation: String, | ||
eventStartDateTime: LocalDateTime, | ||
eventEndDateTime: LocalDateTime, |
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.
인자 네이밍에서 따로 각 인자마다 event를 붙이신 이유가 있을까요 ?
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.
.collection(EVENT_COLLECTION) | ||
.document(eventId) | ||
.update( | ||
mapOf( |
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.
따로 변수로 map을 빼고, update에 인자로 넣는건 어때요 ??
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.
오호라 그럼 이런 경우에는 update
가 적당할까요 ? 아니면 MergeOptions
를 쓰는게 적당한걸까요?
업데이트 되지 않는 항은 eventId
밖에 없긴해요.
따로 변수로 map을 빼고, update에 인자로 넣는건 어때요 ??
좋은 생각입니다!
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.
보통의 상황에서 업데이트를 위한 기능이라면 무조건 update가 낫지 않을까요
좀 과장되게 표현하자면 DiffUtil과 notifyDataSetChanged 차이 같은데요 하하
일부로 MergeOptions을 넣은 경우는
두가지 데이터가 한번에 들어갈 수도 있고, 중복되는 경우를 막기 위한 목적이라
업데이트와 같은 기능은 무조건 update가 맞을 것 같아요
navigateToManagement = navigateToManagement, | ||
) | ||
} | ||
} |
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.
🔥🔥🔥🔥
} | ||
|
||
fun updateEvent() { | ||
if (_eventLocation.value.isEmpty()) { |
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.
작업해놓았았씁니다!
1. 📄 관련된 이슈 및 소개
closed #78
2. 🔥변경된 점
- 관리 화면에서 일정을 클릭할 시, 일정 수정 화면으로 넘어갑니다.
3. 📸 스크린샷(선택)
4. 💡알게된 혹은 궁금한 사항들
Compose Navigation 시 여러 개의 인자를 한 번에 보내기
그냥 한 개 보낼 때 처럼하면 되고, 나중에
arguments
속성에만 새로 넣어주면 된다.레퍼런스
Firebase 데이터 덮어쓰지 않고 수정하기
와 같이
updaate
메소드에map
형식으로 넣어주면 한 번에 수정할 수 있음.레퍼런스