-
Notifications
You must be signed in to change notification settings - Fork 0
북마크 / 도감 UI 수정 #267
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
북마크 / 도감 UI 수정 #267
Conversation
Summary of ChangesHello @pinocchio22, 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! 이 Pull Request는 애플리케이션의 핵심 기능을 대폭 확장하고 개선하는 데 중점을 둡니다. 특히 북마크, 도감 정보 표시, 사용자 인증 및 알림 관리와 관련된 백엔드 통합을 심화하고, 사용자 경험을 향상시키기 위한 새로운 UI/UX 플로우를 도입했습니다. 코드베이스 전반에 걸쳐 데이터 모델, API 연동, 비즈니스 로직, 그리고 프레젠테이션 계층에 이르는 광범위한 변경 사항이 포함되어 있으며, 재사용 가능한 컴포넌트와 모듈화를 통해 향후 개발의 효율성을 높였습니다. 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
|
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.
Code Review
이번 PR은 북마크와 도감 기능의 UI 및 로직을 개선하는 데 중점을 두었습니다. 전반적으로 Reactor를 사용하여 상태 관리를 개선하고, 로그인 상태에 따른 분기 처리를 명확하게 한 점이 인상적입니다. 특히 BookmarkListViewController와 관련 파일들의 리팩토링을 통해 코드의 응집도를 높이고 재사용성을 개선한 점이 좋습니다. 몇 가지 잠재적인 문제점과 개선 사항을 리뷰에 남겼으니 확인 부탁드립니다.
| case .dataTapped(let index): | ||
| let item = currentState.items[index] | ||
| guard let type = item.type.toDictionaryType else { return .empty() } | ||
| return .just(.toNavagate(.detail(type, item.originalId))) |
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.
dataTapped 액션 처리 시 currentState.items 배열에 index로 직접 접근하고 있습니다. 만약 index가 배열의 범위를 벗어날 경우 앱이 강제 종료될 수 있습니다. 안전한 접근을 위해 인덱스 유효성을 검사하는 로직을 추가하는 것이 좋습니다.
| case .dataTapped(let index): | |
| let item = currentState.items[index] | |
| guard let type = item.type.toDictionaryType else { return .empty() } | |
| return .just(.toNavagate(.detail(type, item.originalId))) | |
| case .dataTapped(let index): | |
| guard currentState.items.indices.contains(index) else { return .empty() } | |
| let item = currentState.items[index] | |
| guard let type = item.type.toDictionaryType else { return .empty() } | |
| return .just(.toNavagate(.detail(type, item.originalId))) |
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.
tapped intdex가 items 배열 기반이기떄문에 인덱스 에러 날 경우가 x
| mainText: "북마크를 하려면 로그인이 필요해요.", | ||
| ctaText: "로그인 하기", | ||
| cancelText: "취소", | ||
| ctaAction: { print("로그인 화면으로 이동") }, |
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.
bindBookmarkButton 메서드 내에서 로그인이 필요할 때 표시되는 GuideAlertFactory의 ctaAction이 print문으로 구현되어 있어 실제 로그인 화면으로 이동하지 않습니다. 사용자 경험을 위해 실제 로그인 화면으로 이동하는 로직을 구현해야 합니다.
BookmarkListViewController에서 처리하는 방식과 유사하게 DictionaryDetailBaseViewController가 LoginFactory를 의존성으로 주입받도록 수정하고, ctaAction에서 해당 팩토리를 사용하여 로그인 화면으로 전환하는 것을 권장합니다.
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.
수정완료
| private let subLabel = UILabel() | ||
|
|
||
| private let button = CommonButton(style: .normal, title: "북마크하러 가기", disabledTitle: nil) | ||
| public let button = CommonButton(style: .normal, title: "ㅌ 가기", disabledTitle: nil) |
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.
CommonButton의 초기 타이틀이 "ㅌ 가기"로 설정되어 있습니다. 오타로 보이며, 앱 실행 시 잠시 잘못된 텍스트가 보일 수 있습니다. 이후 updateTitle을 통해 올바른 텍스트로 변경되지만, 초기값도 의미 있는 값(예: "북마크하러 가기")으로 설정하는 것이 좋습니다.
| public let button = CommonButton(style: .normal, title: "ㅌ 가기", disabledTitle: nil) | |
| public let button = CommonButton(style: .normal, title: "북마크하러 가기", disabledTitle: nil) |
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.
수정완료
…le/MLS-iOS into fix/#264-Bookmark-Dictionary-UI # Conflicts: # MLS/Presentation/DictionaryFeature/DictionaryFeature/DictionaryDetail/DictionaryDetailBaseViewController.swift # MLS/Presentation/DictionaryFeature/DictionaryFeature/DictionaryList/DictionaryListViewController.swift
…le/MLS-iOS into fix/#264-Bookmark-Dictionary-UI
…kmark-Dictionary-UI # Conflicts: # MLS/Data/Data/Repository/UserDefaultsRepositoryImpl.swift # MLS/Presentation/DictionaryFeature/DictionaryFeature/DictionaryDetail/DictionaryDetailBaseViewController.swift # MLS/Presentation/DictionaryFeature/DictionaryFeature/DictionaryDetail/Item/ItemDictionaryDetailViewController.swift # MLS/Presentation/DictionaryFeature/DictionaryFeature/DictionaryDetail/Monster/MonsterDictionaryDetailViewController.swift # MLS/Presentation/DictionaryFeature/DictionaryFeature/DictionaryList/DictionaryListFactoryImpl.swift # MLS/Presentation/DictionaryFeature/DictionaryFeature/DictionaryList/DictionaryListReactor.swift # MLS/Presentation/DictionaryFeature/DictionaryFeature/DictionaryList/DictionaryListViewController.swift # MLS/Presentation/DictionaryFeature/DictionaryFeature/DictionarySearch/DictionarySearchViewController.swift
📌 이슈
✅ 작업 사항