[UI/#263] 4차 스프린트 담당 UI 구현합니다. #264
Conversation
|
""" WalkthroughThe changes introduce new UI components and refactor the diary writing screen. A dynamic floating action button (FAB) and a custom send button are added, with conditional rendering based on keyboard and entry states. The top bar is reworked, exit dialog logic is added, and string resources are updated for clarity and new dialog flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WriteDiaryScreen
participant ViewModel
participant UI Components
User->>WriteDiaryScreen: Opens diary screen
WriteDiaryScreen->>UI Components: Renders TopBar, FAB, Entries List
User->>UI Components: Clicks Add FAB
UI Components->>WriteDiaryScreen: onClickAdd()
WriteDiaryScreen->>ViewModel: Add new entry
User->>UI Components: Clicks Send in TopBar
UI Components->>WriteDiaryScreen: onClickSend()
WriteDiaryScreen->>ViewModel: Handle send logic
User->>WriteDiaryScreen: Presses back
WriteDiaryScreen->>ViewModel: Check entries
alt Any entry not blank
WriteDiaryScreen->>UI Components: Show exit dialog
User->>UI Components: Confirm exit
UI Components->>WriteDiaryScreen: onConfirmExitDialog()
WriteDiaryScreen->>ViewModel: Update exit dialog state
else All entries blank
WriteDiaryScreen->>Navigation: Navigate back
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
MoonsuKang
changed the title
MoonsuKang
changed the base branch from
refactor/#260-navigation-structure
to
develop
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/button/SendButton.kt (1)
25-40: Consider adding accessibility support.The button lacks accessibility considerations. Consider adding a content description for screen readers.
You could add a contentDescription parameter:
@Composable fun SendButton( modifier: Modifier = Modifier, onClick: () -> Unit, + contentDescription: String? = null, ) { // ... Box( modifier = modifier .sizeIn(minWidth = 48.dp, minHeight = 48.dp) .clickable( interactionSource = interactionSource, indication = null, onClick = onClick, ) + .semantics { + contentDescription?.let { this.contentDescription = it } + }, contentAlignment = Alignment.Center, ) {Don't forget to import:
+import androidx.compose.ui.semantics.contentDescription +import androidx.compose.ui.semantics.semanticsapp/src/main/java/com/sopt/clody/presentation/ui/writediary/component/topbar/WriteDiaryTopBar.kt (1)
31-37: Consider using a more appropriate icon resource nameThe icon resource
ic_nickname_backseems to be named for a nickname-related screen, but it's being used in the diary writing context. Additionally, the content description is hardcoded.Consider renaming the icon resource to be more generic (e.g.,
ic_back_arrow) or create a diary-specific icon. Also, move the content description to string resources:IconButton(onClick = onClickBack) { Icon( - painter = painterResource(id = R.drawable.ic_nickname_back), - contentDescription = "뒤로가기", + painter = painterResource(id = R.drawable.ic_back_arrow), + contentDescription = stringResource(R.string.content_description_back), tint = ClodyTheme.colors.gray01, ) }app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/button/AddDiaryEntryFAB.kt (1)
86-91: Move hardcoded text to string resourcesThe text "추가하기" is hardcoded and should be moved to string resources for better localization support.
Text( - text = "추가하기", + text = stringResource(R.string.write_diary_add_entry), color = contentColor, style = ClodyTheme.typography.body2SemiBold, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/main/java/com/sopt/clody/presentation/ui/component/toast/ClodyToastMessage.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/button/AddDiaryEntryFAB.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/button/SendButton.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/textfield/WriteDiaryTextField.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/topbar/WriteDiaryTopBar.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt(2 hunks)app/src/main/res/values/strings.xml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/topbar/WriteDiaryTopBar.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/button/SendButton.kt (1)
SendButton(17-41)
app/src/main/java/com/sopt/clody/presentation/ui/component/toast/ClodyToastMessage.kt (1)
app/src/main/java/com/sopt/clody/ui/theme/Theme.kt (1)
CLODYTheme(7-12)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (14)
app/src/main/res/values/strings.xml (2)
66-75: LGTM! String changes align with UI flow improvements.The string updates properly reflect the terminology shift from "saving" to "sending" diary entries, and the new exit dialog strings support the enhanced user experience with exit confirmation.
83-83: Consistent validation message update.The toast message update aligns well with the new "sending" workflow and provides clear feedback to users about form validation requirements.
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt (2)
57-58: Good state management pattern.The
showExitDialogstate follows the same pattern as other dialog states in the ViewModel with a private setter, maintaining proper encapsulation.
168-170: Consistent with existing dialog management.The
updateShowExitDialogmethod is consistent with other similar methods in the class for managing dialog visibility states.app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/textfield/WriteDiaryTextField.kt (1)
145-157: Excellent conditional UI implementation.The conditional rendering of the remove button based on
isRemovableimproves component flexibility while maintaining consistent layout spacing with the Spacer fallback. This follows Compose best practices for conditional UI elements.app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/topbar/WriteDiaryTopBar.kt (1)
19-51: LGTM! Well-structured top bar componentThe component is well-designed with proper Material3 API usage, theme integration, and clear separation of navigation and action areas. The window insets handling is correctly implemented.
app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/button/AddDiaryEntryFAB.kt (1)
31-95: Excellent adaptive FAB implementationThe FAB component shows great attention to UX with:
- Smooth animations between keyboard states
- Clear visual feedback for disabled state
- Proper IME padding consideration
- Clean separation of circular and expanded states
The use of
BoxScopeextension is a good architectural choice for positioning control.app/src/main/java/com/sopt/clody/presentation/ui/component/toast/ClodyToastMessage.kt (2)
42-51: Good improvements to toast layout consistencyThe changes from
wrapContentHeight()to a fixedheight(42.dp)with centered content alignment ensure consistent toast appearance across different content lengths. The centered horizontal arrangement in the Row also improves visual balance.
68-81: Preview enhancement with proper themingGood improvement to wrap the preview in
CLODYThemeand use actual theme colors and icon resources. This provides a more accurate preview representation.app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt (5)
94-101: Smart exit confirmation logicExcellent UX consideration - only showing the exit dialog when there's actual content prevents unnecessary friction for users who haven't started writing.
179-183: Proper keyboard visibility detectionGood implementation of keyboard visibility detection using
WindowInsets.ime. This enables responsive UI adjustments like the FAB transformation.
319-344: Well-implemented stacked toast messagesThe vertical stacking of toast messages with
imePadding()ensures they remain visible above the keyboard. The use ofzIndex(1f)in the parent Box ensures proper layering.
53-147: Excellent state hoisting implementationThe refactoring successfully moves state management to the Route level, improving:
- Component reusability
- Separation of concerns
- Testability
- Clear data flow
This aligns well with modern Compose best practices.
121-129:Details
✅ Verification successful
Consider edge case in validation logic
The validation checks if all warnings are false and then checks for empty fields. However, there might be a race condition where
showWarningshasn't been updated yet when this check runs.Verify that
validateEntries()synchronously updatesshowWarningsbefore the check:
🏁 Script executed:
#!/bin/bash # Search for validateEntries implementation to ensure synchronous behavior ast-grep --pattern 'fun validateEntries() { $$$ }'Length of output: 776
🏁 Script executed:
#!/bin/bash # Locate showWarnings and related state definitions rg -n "showWarnings" app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt # Inspect validation methods and state updates sed -n '1,200p' app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.ktLength of output: 6083
The
validateEntries()method and its helpervalidateEntry()run entirely on the JVM main thread and synchronously update the_showWarningslist before returning. There is no coroutine or async boundary in between. Therefore, by the time you run yourif (showWarnings.all { !it })check in the composable, all warning flags have already been updated. You can safely remove the “race condition” caveat from your review.
| contentAlignment = Alignment.Center, | ||
| ) { | ||
| Text( | ||
| text = "보내기", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use string resource instead of hardcoded text.
The button text "보내기" should use a string resource for better maintainability and localization support.
Apply this change:
- text = "보내기",
+ text = stringResource(R.string.write_diary_dialog_confirm_option),Add the import:
+import androidx.compose.ui.res.stringResource📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| text = "보내기", | |
| // Add at the top of the file alongside your existing imports | |
| import androidx.compose.ui.res.stringResource | |
| @Composable | |
| fun SendButton( | |
| onClick: () -> Unit, | |
| enabled: Boolean = true, | |
| modifier: Modifier = Modifier | |
| ) { | |
| Button( | |
| onClick = onClick, | |
| enabled = enabled, | |
| modifier = modifier, | |
| // … other parameters … | |
| - text = "보내기", | |
| + text = stringResource(R.string.write_diary_dialog_confirm_option), | |
| // … any trailing commas or other args … | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/button/SendButton.kt
at line 36, replace the hardcoded button text "보내기" with a reference to a string
resource. Define the string "보내기" in the appropriate strings.xml file and update
the code to use this string resource via context.getString or the Compose
stringResource function. Also, add the necessary import for stringResource if
using Compose.
SYAAINN
left a comment
SYAAINN
left a comment
There was a problem hiding this comment.
수고하셨습니다! 중간중간 고정 dp 값들이 많이 보이는데 UI 대응면이 조금 걱정됩니다 ㅜ 괜찮을까요?
SYAAINN
left a comment
SYAAINN
left a comment
There was a problem hiding this comment.
이 PR 에서 일기 작성화면 LazyColumn도 적용하면 좋을것 같습니다!! 디자인 측에서 들어온 수정사항이에요
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt (2)
105-132: Consider extracting complex validation logic.The validation logic in
onClickCompleteis becoming complex and could benefit from extraction to improve readability and testability.Consider extracting the validation logic:
onClickComplete = { - viewModel.validateEntries() - if (showWarnings.all { !it }) { - if (entries.size > 1 && entries.any { it.isEmpty() }) { - viewModel.updateShowEmptyFieldsMessage(true) - } else { - AmplitudeUtils.trackEvent(AmplitudeConstraints.WRITING_DIARY_COMPLETE) - viewModel.updateShowDialog(true) - } - } + viewModel.handleCompleteAction() },And move this logic to the ViewModel as a single method.
326-351: Consider accessibility improvements for stacked toasts.The Column-based approach for stacking toasts works well, but consider adding semantic properties for better accessibility support with screen readers.
Consider adding semantics for better accessibility:
Column( modifier = modifier + .semantics { + contentDescription = "Toast messages" + }, horizontalAlignment = Alignment.CenterHorizontally, ) {Also consider adding a small vertical spacing between toasts if both can appear simultaneously:
Column( modifier = modifier, horizontalAlignment = Alignment.CenterHorizontally, + verticalArrangement = Arrangement.spacedBy(8.dp), ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (12)
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt (12)
11-19: New imports look appropriate for the UI refactoring.The new imports support the enhanced functionality including WindowInsets handling, IME padding, keyboard detection, preview support, zIndex layering, and new UI components. The imports are well-organized and necessary for the implemented features.
Also applies to: 30-30, 33-33, 35-35, 43-43, 47-47, 52-52
65-75: Improved state management with proper separation of concerns.The addition of new state properties like
showExitDialog,showFailureDialog, andfailureMessagefollows good state management practices by keeping UI state separate and observable.
183-185: Keyboard visibility detection is implemented correctly.The keyboard visibility detection using WindowInsets.ime is the recommended approach for handling keyboard state in Compose.
205-209: Good implementation of focus clearing on outside clicks.The clickable modifier with null indication and custom interaction source properly handles clearing focus when clicking outside text fields without visual feedback.
227-246: Excellent use of WindowInsets for keyboard handling.The LazyColumn properly handles both navigation bars and IME insets, ensuring content remains accessible when the keyboard appears.
248-286: Dialogs are properly conditionally rendered.All dialog components are correctly placed and conditionally rendered based on their respective state flags. The exit dialog implementation looks comprehensive.
294-309: Toast message stacking implementation looks solid.The Box with proper alignment and zIndex for toast messages, combined with imePadding, ensures toasts remain visible and properly positioned even when the keyboard is shown.
354-388: Well-structured preview with comprehensive test data.The preview function provides good test data including different states for warnings and various UI flags, which will be helpful for UI development and testing.
288-292:Details
✅ Verification successful
Verify AddDiaryEntryFAB component behavior.
The FAB component receives proper state for keyboard visibility and max entries. Ensure it handles disabled state correctly when max is reached.
🏁 Script executed:
#!/bin/bash # Check the AddDiaryEntryFAB implementation cat app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/button/AddDiaryEntryFAB.ktLength of output: 3666
No changes needed: AddDiaryEntryFAB behavior is correct
The FAB usesisMaxReachedto switch itscontainerColorand disables click handling viaclickableWithoutRipple(enabled = !isMaxReached). Keyboard visibility is animated correctly.
97-104:Details
❓ Verification inconclusive
Verify the exit dialog logic and user experience.
The back button logic now shows an exit dialog when entries contain text, which is good UX. However, consider edge cases where users might have whitespace-only entries.
Consider enhancing the condition to handle whitespace-only entries:
- if (entries.any { it.isNotBlank() }) { + if (entries.any { it.trim().isNotEmpty() }) {Also verify that the AmplitudeUtils.trackEvent call doesn't block the UI thread:
🏁 Script executed:
#!/bin/bash # Check if AmplitudeUtils.trackEvent is async or could block UI ast-grep --pattern 'class AmplitudeUtils { $$$ fun trackEvent($$$) { $$$ } $$$ }'Length of output: 95
Exit dialog logic handles whitespace-only entries correctly
Kotlin’s
CharSequence.isNotBlank()already returns false for empty or whitespace-only strings, so no change toif (entries.any { it.isNotBlank() }) { … }is needed. You may add an inline comment if you’d like to clarify its whitespace‐trimming behavior.
Verify AmplitudeUtils.trackEvent is non-blocking
• Please confirm thatAmplitudeUtils.trackEvent(…)runs asynchronously or otherwise won’t block the UI thread.
235-244:Details
✅ Verification successful
Verify the WriteDiaryTextField isRemovable logic.
The
isRemovable = entries.size > 1logic ensures users can't remove the last entry, which is good UX. Verify this component handles the conditional rendering properly.
🏁 Script executed:
#!/bin/bash # Check the WriteDiaryTextField implementation for isRemovable handling ast-grep --pattern 'fun WriteDiaryTextField( $$$ isRemovable: Boolean, $$$ ) { $$$ }'Length of output: 95
🏁 Script executed:
#!/bin/bash # Locate the WriteDiaryTextField composable definition rg -n "fun WriteDiaryTextField" -A10Length of output: 1476
🏁 Script executed:
#!/bin/bash # Search for usage of isRemovable within WriteDiaryTextField implementation rg -n "isRemovable" -C5 app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/textfield/WriteDiaryTextField.ktLength of output: 1156
WriteDiaryTextField conditional removal logic is correctly handled
The
isRemovableflag is used in WriteDiaryTextField (at line 145) to conditionally render the remove button only whenisRemovable == true. Since you’re passingentries.size > 1from the caller, the last entry cannot be removed—exactly as intended. No further changes needed.
189-193:Details
✅ Verification successful
Verify the WriteDiaryTopBar component implementation.
The new topBar component looks clean, but ensure it properly handles state and accessibility.
🏁 Script executed:
#!/bin/bash # Check the WriteDiaryTopBar implementation cat app/src/main/java/com/sopt/clody/presentation/ui/writediary/component/topbar/WriteDiaryTopBar.ktLength of output: 1868
🏁 Script executed:
#!/bin/bash # Locate and inspect the SendButton implementation for accessibility semantics rg -n "fun SendButton" -R app/src/main/javaLength of output: 450
🏁 Script executed:
#!/bin/bash # Locate and print the SendButton implementation for accessibility and state handling review send_btn_file=$(fd -e kt SendButton.kt | head -n1) echo "SendButton file: $send_btn_file" sed -n '1,200p' "$send_btn_file"Length of output: 1885
WriteDiaryTopBar implementation verified
- The back IconButton provides a descriptive
contentDescription("뒤로가기") for screen readers.SendButtonmanages its pressed state viaMutableInteractionSourceand updates its text color for visual feedback.- Using
Modifier.clickableon the Box around the “보내기” Text automatically adds the appropriate button semantics and announces the label.TopAppBarapplies safe window insets and a clear container color.No further changes required.
| fun WriteDiaryScreen( | ||
| viewModel: WriteDiaryViewModel, | ||
| isLoading: Boolean, | ||
| entries: List<String>, | ||
| showWarnings: List<Boolean>, | ||
| showLimitMessage: Boolean, | ||
| showEmptyFieldsMessage: Boolean, | ||
| showDeleteBottomSheet: Boolean, | ||
| entryToDelete: Int, | ||
| allFieldsEmpty: Boolean, | ||
| showDialog: Boolean, | ||
| onClickBack: () -> Unit, | ||
| onCompleteClick: () -> Unit, | ||
| onClickAdd: () -> Unit, | ||
| onClickRemove: (Int) -> Unit, | ||
| onConfirmDelete: () -> Unit, | ||
| onDismissDelete: () -> Unit, | ||
| onTextChange: (Int, String) -> Unit, | ||
| onClickComplete: () -> Unit, | ||
| onConfirmDialog: () -> Unit, | ||
| onDismissDialog: () -> Unit, | ||
| onDismissLimitMessage: (Boolean) -> Unit, | ||
| onDismissEmptyFieldsMessage: (Boolean) -> Unit, | ||
| showFailureDialog: Boolean, | ||
| failureMessage: String, | ||
| showExitDialog: Boolean, | ||
| onDismissFailureDialog: () -> Unit, | ||
| onDismissExitDialog: () -> Unit, | ||
| onConfirmExitDialog: () -> Unit, | ||
| year: Int, | ||
| month: Int, | ||
| day: Int, | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reducing the number of function parameters.
The WriteDiaryScreen composable has many parameters (17+), which can make it difficult to maintain and test. Consider grouping related parameters into data classes.
Consider creating data classes to group related parameters:
data class WriteDiaryScreenState(
val isLoading: Boolean,
val entries: List<String>,
val showWarnings: List<Boolean>,
val showLimitMessage: Boolean,
val showEmptyFieldsMessage: Boolean,
val showDeleteBottomSheet: Boolean,
val showDialog: Boolean,
val showFailureDialog: Boolean,
val showExitDialog: Boolean,
val failureMessage: String
)
data class WriteDiaryScreenCallbacks(
val onClickBack: () -> Unit,
val onClickAdd: () -> Unit,
val onClickRemove: (Int) -> Unit,
// ... other callbacks
)This would improve maintainability and make the composable easier to test.
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryScreen.kt
around lines 153 to 181, the WriteDiaryScreen function has too many parameters,
making it hard to maintain and test. Refactor by creating data classes to group
related parameters: one for the UI state (e.g., isLoading, entries,
showWarnings, showLimitMessage, showEmptyFieldsMessage, showDeleteBottomSheet,
showDialog, showFailureDialog, showExitDialog, failureMessage) and another for
the callbacks (e.g., onClickBack, onClickAdd, onClickRemove, onConfirmDelete,
onDismissDelete, onTextChange, onClickComplete, onConfirmDialog,
onDismissDialog, onDismissLimitMessage, onDismissEmptyFieldsMessage,
onDismissFailureDialog, onDismissExitDialog, onConfirmExitDialog). Then update
the WriteDiaryScreen function signature to accept these data classes instead of
individual parameters.
키보드 상태에 따라 FAB UI가 전환되도록 구현 최대 항목 수 도달 시 FAB 비활성화 및 색상 변경 처리
- 모든 다이얼로그 UI를 Route → Screen으로 이동하여 책임 분리 명확화 - WriteDiaryTopBar, AddDiaryEntryFAB 적용 - Box로 Screen content를 감싸고 FAB/Toast 등을 배치할 수 있도록 구조 개선
MoonsuKang
force-pushed
the
ui/#263-fourth-sprint-ui
branch
from
ed9217f to
e9d0046
Compare
📌 ISSUE
closed #263
📄 Work Description
✨ PR Point
여긴 mavericks 적용 안하고 우선 기존 구조를 유지하겠습니다?
📸 ScreenShot/Video
4spr.mp4
Summary by CodeRabbit
New Features
Improvements