[Feat] 제휴 정보 토글 default상태 변경 및 Map 관련 코드 리팩토링 #417
Conversation
… into feat/replace-map-toggle-default # Conflicts: # gradle/libs.versions.toml
…보, 학과 입력 바텀시트 상태 변수 분리
Summary of ChangesHello @kangyuri1114, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a more robust and user-friendly map experience by implementing dynamic default filtering for partnerships based on user department information. It significantly refactors the underlying Compose code, separating UI concerns from business logic and centralizing state management within the ViewModel. These changes result in a more maintainable codebase and a more intuitive interaction flow for users navigating partnership information on the map. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
app/src/main/java/com/eatssu/android/presentation/map/MapFragmentView.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/eatssu/android/presentation/map/MapFragmentView.kt
Outdated
Show resolved
Hide resolved
HI-JIN2
left a comment
HI-JIN2
left a comment
There was a problem hiding this comment.
compose 관련(bom, material3) 버전을 업데이트하고 material의존성을 제거하려고 했는데, 어제 리뷰v2 머지된 후에 rebase해보니 리뷰 V2에서는 material3가 아닌 material의 Icon을 사용하고 있더라구요
그래서 우선은 compose 버전 업데이트 관련은 다시 원래대로 돌려놨습니다
-> 요기 확인하고 material3만 남기도록 수정해서 올리겠습니다!
must한건 없어서 어푸릅 드립니다~! 수고 많았어용 🫶
전반적으로 훨씬 좋아진 것 같아요!! 유리님께서 만족하는 코드였으면 좋겠습니다 ><
app/src/main/java/com/eatssu/android/presentation/map/component/PartnershipToggleItem.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Composable | ||
| private fun MapScreen( |
There was a problem hiding this comment.
p2 저는 internal fun으로 하는 편입니다
- internal fun : UI
- private fun: 내부 컴포넌트
요즘 요렇게 많이 쓰이는 것 같아요. 비슷한 접근제어자라서 제 견해로는 UI 전체인지, 컴포넌트인지를 구분하는 용도로 써도 좋을 것 같아요
There was a problem hiding this comment.
저는 MapScreen / internal fun MapScreen 이렇게 하는 편인데,
전에 유리님께서 route 네이밍을 얘기하셨어서, MapRoute / internal MapScreen 요렇게 절충안을 제안해봅니다!
There was a problem hiding this comment.
internal은 모듈 내에서 접근 가능, private은 파일 내 접근 가능으로 알고 있었습니다
리뷰를 읽고 사실 굳이 internal을 써야 할까 의문이 들었었는데,
곰곰쓰 생각해보니까
하나의 compose 파일 내에 모든 screen이 존재하라는 보장이 없으니(길어지면 보통 분리하니까! 또는 Screen이 여러개라면 별도 파일로 둘 수도 있고) private보다는 internal이 맞겠네요!
내부 컴포넌트는 core컴포넌트가 아닌 이상 보통 한 파일에 두니까 private이 맞을 것 같구요
이부분도 wiki에 적어두면 좋을 거 같아요
그런데 이렇게 정한 이유가 위에 제가 설명한 내용이 맞는지 검토해주시고 동의해주시면 wiki에 적어두겠습니다!
우선 해당 부분 코드는 수정했습니다!
There was a problem hiding this comment.
맞습니다~
근데 또 생각해보니까 내부컴포넌트도 별도의 파일로 분리하고 있지 않나? 라는 생각이 듭니다 🤔
app/src/main/java/com/eatssu/android/presentation/map/MapFragmentView.kt
Outdated
Show resolved
Hide resolved
| val showDepartmentBottomSheet: Boolean = false, | ||
| val showPartnershipBottomSheet: Boolean = false, |
| // Domain 모델(RestaurantType)을 UI 모델(PlaceType)로 변환 | ||
| val placeType = when (representative.restaurantType) { | ||
| RestaurantType.CAFE -> PlaceType.CAFE | ||
| RestaurantType.RESTAURANT -> PlaceType.RESTAURANT | ||
| RestaurantType.PUB -> PlaceType.PUB | ||
| } |
There was a problem hiding this comment.
이거 한 enum을 core 모듈에 두고 쓰는건 어떤가요? 같은 네이밍인데 굳이? 라는 생각이 듭니다
There was a problem hiding this comment.
저는 사실 Domain <> UI 모델을 습관적으로 분리해서 정의하고 mapping하는 과정을 꼭 거치게 작업을 하는데요!
그 이유는 아시는 것처럼 내부 모델과 서버의 모델(저장소 모델) 중 둘 중 하나의 구조가 변경될 때 mapper만 수정 가능하다는 확장성 때문입니다!
말씀처럼 이 부분은 같은 모양 같은 이름 같은 쓰임새라 core로 두고 써도 되긴 할 것 같아요
근데 앞으로 서버 모델이나 우리 ui모델이 변할거라는 보장이 없기때문에(물론 이 부분은 변할 거 같진 않긴해요 ㅎㅎ..) 굳이 이미 나눈 것을 다시 core로 합치는 과정이 필요할까?! 라는 의문이 듭니다 ㅎㅎ!
There was a problem hiding this comment.
이거는 저도 바이블적인 예시를 보고 말하는게 아니라서 제 개인적인 견해인데요, 안드팀 내부에서만 쓰이는 enum이 아니라 서버<->클라 전역에서 쓰이는 enum이라서 core에 두고 쓰는게 좋지 않을까? 라는 생각이 들었어요. enum은 보통 서버에서 정의한대로 쓰긴하지만, 팀 내에서 협약한 사항이기에 안바뀌거나 바뀌어도 다같이 바뀌겠죵? 그에 대한 검증인 mapping을 안하는게 코드를 읽고 유지보수하는 입장에서 편하지 않을까의 이유입니다! 이러한 예로 Restraunt랑 Time이 그렇게 쓰이고 있죵
(이러한 의도로 enum을 코어모듈로 옮겼습니다)
There was a problem hiding this comment.
음 이건 정답이 없는 거 같은데 어떻게 하고 이 PR을 머지 해야할까요
사실 며칠동안 생각했는데 저도 답을 못내리겠네요
There was a problem hiding this comment.
사실 클린 아키텍처만 두고 논한다면 Domain enum을 UI에서 쓰는 건 좋진 않은 방식같은데
유지보수성을 생각하면 진님 말도 맞는 말이라서요
관점에 따라 다를거같아요
kangyuri1114
changed the title
* fix: DTO 파일 이동에 따른 Proguard Rule 수정 * fix: Generics 관련 정보가 유실되는 TypeToken 대신 Java Class 사용 * release: 3.1.8(46) * refactor: unused import 삭제 * fix: 제미니 대응
|
PlayType enum 부분 의견 주신 것 제외하고는 모두 반영 완료했습니다 ~ @HI-JIN2 그리고 한번 더 테스트하다가 제휴 토글을 바꿨을 때 "특정 식당에 대한 제휴정보 바텀시트"가 또 올라오는 오류를 수정했습니다! |
Summary
학과 미입력 시 전체 제휴정보를 default, 입력 시 학과 제휴정보를 default로 화면이 보여집니다.
Map관련 Compose 코드 리팩토링 및 View-ViewModel 관심사 분리
Describe your changes
Screen_recording_20251125_105207.mp4
영상은 debug모드라 지도가 잘 나오지 않는데 참고부탁드립니다
[작업 리스트]
기능적으로 바뀐 것
코드적으로 바뀐 것
6. compose view와 router를 분리해 관심사 분리를 했습니다. screen에는 data만 전달하도록해 관심사를 분리했습니다.
7. init 내부에서 전체 제휴 load하던 코드는 삭제하고 토글 선택 값에 따라 각각에 맞는 제휴 정보를 불러오도록 수정했습니다.
8. 사용되지 않았던 MapState를 일부 삭제하고 SheetState를 뷰모델에서 관리하도록 하기 위해 sheetstate, filtertype을 mapState로 추가했습니다.
9. 하나의 sheetState로 관리되던 제휴정보, 학과 입력 바텀시트 상태 변수 분리
10. 도메인의 placeType을 UI의 placeType으로 변환하는 코드를 View -> ViewModel로 이동했습니다.
11. 필터 토글 상태 관리 및 이벤트 로거 코드는 ViewModel로 이동시켰습니다.
Issue
To reviewers
그래서 우선은 compose 버전 업데이트 관련은 다시 원래대로 돌려놨습니다