Feat/#12 home 구현 #16
Feat/#12 home 구현 #16
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @rhkrwngud445, 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 the foundational UI for the home screen, focusing on displaying contact information and monthly contact schedules. It lays the groundwork for future features like drag-and-drop editing and actual data integration, which are explicitly noted for separate follow-up PRs.
Highlights
- Initial Home Screen UI: I've implemented the core user interface for the home screen, including the layout for "This Month's People" and "My People" sections, complete with a background image, navigation elements, and text.
- New Data Models for Contacts: I've added ContactFrequency, ContactSummary, and MonthlyContact data models to structure and represent contact information and upcoming contact schedules within the application.
- Reusable UI Components: I've created several modular Compose UI components such as AddContactButton, AddContactImage, ContactItem, and MyContacts to efficiently build and manage the contact display on the home screen.
- Updated Minimum SDK: The project's minimum SDK version has been raised from 24 to 27 in build.gradle.kts.
- UI Utility Extensions: I've introduced new Modifier extension functions, onNoRippleClick for custom click behavior and dropShadow for enhanced visual effects, to improve UI interactivity and aesthetics.
- Resource Expansion: I've added a comprehensive set of new drawable assets for various icons and a user image, alongside new string resources to support the newly implemented home screen UI.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request implements the home screen UI. The implementation is quite comprehensive, including various components for displaying contacts. I've found a critical bug in the onNoRippleClick modifier extension that prevents click events from being handled. Additionally, there are several areas for improvement regarding performance, code structure, and accessibility. For instance, some data models create formatters repeatedly, and the layout logic for contacts in MyContacts.kt is very complex and could be simplified with a custom layout. I've also pointed out several places where hardcoded values and missing content descriptions should be addressed. Great work on the overall feature, looking forward to the data connection part!
| fun Modifier.onNoRippleClick(onClick: () -> Unit): Modifier = | ||
| this then | ||
| Modifier.clickable( | ||
| onClick = {}, |
There was a problem hiding this comment.
The onClick parameter of clickable is hardcoded to an empty lambda {}, which means the onClick lambda passed to onNoRippleClick is never executed. This is a critical bug.
| onClick = {}, | |
| onClick = onClick, |
| when (contactsWithPage[page].count()) { | ||
| 1 -> { | ||
| Column { | ||
| Spacer(modifier = Modifier.height(166.dp)) | ||
| Row( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.Center, | ||
| ) { | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][0], | ||
| onClick = onContactClick, | ||
| ) | ||
| Spacer(modifier = Modifier.width((60 - OVERFLOW_WIDTH_OF_CONTACT_ITEM_BY_NAME_TEXT).dp)) | ||
| AddContactButton( | ||
| onClick = onAddContactClick, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 2 -> { | ||
| Column( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| ) { | ||
| Spacer(modifier = Modifier.height(112.dp)) | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][0], | ||
| onClick = onContactClick, | ||
| ) | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| Row { | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][1], | ||
| onClick = onContactClick, | ||
| ) | ||
| Spacer(modifier = Modifier.width((118 - OVERFLOW_WIDTH_OF_CONTACT_ITEM_BY_NAME_TEXT).dp)) | ||
| AddContactButton( | ||
| onClick = onAddContactClick, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 3 -> { | ||
| Column( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| ) { | ||
| Spacer(modifier = Modifier.height(76.dp)) | ||
| Box { | ||
| ContactItem( | ||
| modifier = Modifier.align(Alignment.TopCenter), | ||
| contactSummary = contactsWithPage[page][0], | ||
| onClick = onContactClick, | ||
| ) | ||
| Row(modifier = Modifier.padding(top = 92.dp, bottom = 78.dp)) { | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][1], | ||
| onClick = onContactClick, | ||
| ) | ||
| Spacer(modifier = Modifier.width((141 - OVERFLOW_WIDTH_OF_CONTACT_ITEM_BY_NAME_TEXT).dp)) | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][2], | ||
| onClick = onContactClick, | ||
| ) | ||
| } | ||
| AddContactButton( | ||
| modifier = Modifier.align(Alignment.BottomCenter), | ||
| onClick = onAddContactClick, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 4 -> { | ||
| Column( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| ) { | ||
| Spacer(modifier = Modifier.height(68.dp)) | ||
| Box { | ||
| ContactItem( | ||
| modifier = Modifier.align(Alignment.TopCenter), | ||
| contactSummary = contactsWithPage[page][0], | ||
| onClick = onContactClick, | ||
| ) | ||
| Row(modifier = Modifier.padding(top = 62.dp)) { | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][1], | ||
| onClick = onContactClick, | ||
| ) | ||
| Spacer(modifier = Modifier.width((138 - OVERFLOW_WIDTH_OF_CONTACT_ITEM_BY_NAME_TEXT).dp)) | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][2], | ||
| onClick = onContactClick, | ||
| ) | ||
| } | ||
| } | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| Row { | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][3], | ||
| onClick = onContactClick, | ||
| ) | ||
| Spacer(modifier = Modifier.width((51 - OVERFLOW_WIDTH_OF_CONTACT_ITEM_BY_NAME_TEXT).dp)) | ||
| AddContactButton( | ||
| onClick = onAddContactClick, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 5 -> { | ||
| Column( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| ) { | ||
| Spacer(modifier = Modifier.height(68.dp)) | ||
| Box { | ||
| ContactItem( | ||
| modifier = Modifier.align(Alignment.TopCenter), | ||
| contactSummary = contactsWithPage[page][0], | ||
| onClick = onContactClick, | ||
| ) | ||
| Row(modifier = Modifier.padding(top = 62.dp)) { | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][1], | ||
| onClick = onContactClick, | ||
| ) | ||
| Spacer(modifier = Modifier.width((138 - OVERFLOW_WIDTH_OF_CONTACT_ITEM_BY_NAME_TEXT).dp)) | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][2], | ||
| onClick = onContactClick, | ||
| ) | ||
| } | ||
| } | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| Row { | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][3], | ||
| onClick = onContactClick, | ||
| ) | ||
| Spacer(modifier = Modifier.width((51 - OVERFLOW_WIDTH_OF_CONTACT_ITEM_BY_NAME_TEXT).dp)) | ||
| ContactItem( | ||
| contactSummary = contactsWithPage[page][4], | ||
| onClick = onContactClick, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This when block for laying out contacts is very complex and contains a lot of duplicated code. This makes it difficult to maintain and prone to errors. I see the note in the PR description about planning to refactor this, which is great. I'd strongly recommend prioritizing that refactor. A custom Layout composable would be a much cleaner and more scalable solution for arranging the contact items based on their count. This would eliminate the need for hardcoded Spacer values and manual position calculations.
| ) { | ||
| Image( | ||
| painterResource(R.drawable.icon_visual_mail), | ||
| contentDescription = "", |
There was a problem hiding this comment.
This image seems to be decorative. For purely decorative images, the contentDescription should be set to null so that accessibility services can ignore it.
| contentDescription = "", | |
| contentDescription = null, |
| contacts = | ||
| List(6) { | ||
| ContactSummary( | ||
| id = "2003", | ||
| name = "일이삼사오육칠팔구", | ||
| profileImageUrl = "https://search.yahoo.com/search?p=partiendo", | ||
| lastContactedAt = LocalDate.of(2025, 7, 25), | ||
| isContacted = false, | ||
| contactFrequency = ContactFrequency.LOW, | ||
| ) | ||
| }, | ||
| monthlyContacts = emptyList(), |
There was a problem hiding this comment.
Hardcoding test data directly in the HomeRoute is not ideal, even for development purposes. This data should be provided by the HomeViewModel via the uiState. For previews, it's great that you're already providing sample data in HomeScreenPreview. This hardcoded data should be removed and fetched from the ViewModel instead.
| Image( | ||
| modifier = Modifier.onNoRippleClick(onClick = onAlarmClick), | ||
| painter = painterResource(R.drawable.ic_32_bell), | ||
| contentDescription = "", |
There was a problem hiding this comment.
For accessibility, it's important to provide a meaningful contentDescription for images and icons that are interactive. Since this bell icon is clickable, it should have a description like "Notifications". If it were purely decorative, you would set contentDescription = null.
| contentDescription = "", | |
| contentDescription = "Notifications", |
| Box { | ||
| Image( | ||
| painter = painterResource(R.drawable.img_64_user1), | ||
| contentDescription = "", |
There was a problem hiding this comment.
For accessibility, provide a meaningful contentDescription. For example: "Profile image of ${contactSummary.name}".
| contentDescription = "", | |
| contentDescription = "Profile image of ${contactSummary.name}", |
| ContactFrequency.MIDDLE -> painterResource(R.drawable.ic_visual_24_emoji_50) | ||
| ContactFrequency.HIGH -> painterResource(R.drawable.ic_visual_24_emoji_100) | ||
| }, | ||
| contentDescription = "", |
There was a problem hiding this comment.
This image appears to be decorative. For decorative images, set contentDescription to null to improve accessibility by allowing screen readers to skip it.
| contentDescription = "", | |
| contentDescription = null, |
| Spacer(modifier = Modifier.width(2.dp)) | ||
| Image( | ||
| painter = painterResource(R.drawable.ic_12_check), | ||
| contentDescription = "", |
There was a problem hiding this comment.
This checkmark image seems to indicate a status. For accessibility, it should have a contentDescription that describes its meaning, for example "Contacted". If it's purely decorative alongside the date text, it can be set to null.
| contentDescription = "", | |
| contentDescription = "Contacted", |
| private fun getDaysBetween(): Long { | ||
| val formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd") | ||
| val targetDate = LocalDate.parse(nextContactAt, formatter) | ||
| val today = LocalDate.now() | ||
| return ChronoUnit.DAYS.between(today, targetDate) | ||
| } |
There was a problem hiding this comment.
This function has a couple of issues:
- A new
DateTimeFormatteris created on each call, which is inefficient. It's better to declare it as a top-level constant. - Using
LocalDate.now()makes this logic hard to test with a fixed date. It's better to pass the current date as a parameter to this function to improve testability.
| val dDay: String | ||
| get() { | ||
| val daysBetween = getDaysBetween() | ||
| return when { | ||
| daysBetween == 0L -> "D-day" | ||
| daysBetween > 0L -> "D-$daysBetween" | ||
| else -> "D+${-daysBetween}" // 과거 날짜 | ||
| } | ||
| } | ||
|
|
||
| val isDDay: Boolean | ||
| get() = getDaysBetween() == 0L |
There was a problem hiding this comment.
The getDaysBetween() function is called in both dDay and isDDay getters. This is inefficient as the calculation is performed twice if both properties are accessed. Consider calculating it once and reusing the result.
For example, you could have a private property:
private val daysBetween by lazy { getDaysBetween() }And then use it in your getters.
작업 내용
홈 UI 구현
확인 방법
각 컴포넌트 별 Preview 및 앱 구동
참고 사항
내 사람들에 해당하는 뷰가 Drag and Drop으로 편집 기능이 필요합니다. 현재는 단순히 Linear 형태로 구성해서, 좌표 형식의 구성으로 바꾸어야 할 것 같습니다..
조금 공수가 있을 것 같아 별도의 PR로 작업해볼게요!
데이터 연결 작업 또한 별도 PR로 작업하겠습니다!
관련 이슈