SCRUM-232 design: add setting view#31
Open
gdaegeun539 wants to merge 6 commits intoproject-lyrics:developfrom
Open
SCRUM-232 design: add setting view#31gdaegeun539 wants to merge 6 commits intoproject-lyrics:developfrom
gdaegeun539 wants to merge 6 commits intoproject-lyrics:developfrom
Conversation
- Add HomeGraph, NoteSearchGraph, and MyPageGraph routes - Wire up navigation callbacks for Setting and UserInfo screens - Update scaffold bottom bar to use graph routes
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe changes introduce a new Settings and User Info flow within the MyPage section, including refactored navigation with nested graph routes (HomeGraph, NoteSearchGraph, MyPageGraph), a new centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
app/src/main/java/com/lyrics/feelin/presentation/view/mypage/userinfo/UserInfoScreen.kt (2)
41-44: Simplify hardcoded data declarations.Using
remember { mutableStateOf(...) }for static mock data adds unnecessary overhead and suggests the values could change when they won't. For design placeholders, simplevaldeclarations suffice.Proposed simplification
- val loginInfoItem by remember { mutableStateOf(LoginInfoConfigs.kakao) } - val uid by remember { mutableStateOf("8ca0fd81-fd03-438c-8730-c6c4e7ef4aa9-8ca0fd81-fd03") } - val gender by remember { mutableStateOf("여성") } - val birthYear by remember { mutableStateOf("2000") } + // TODO: Replace with actual user data from ViewModel + val loginInfoItem = LoginInfoConfigs.kakao + val uid = "8ca0fd81-fd03-438c-8730-c6c4e7ef4aa9-8ca0fd81-fd03" + val gender = "여성" + val birthYear = "2000"When real data integration is added, these should be hoisted to a ViewModel and observed as state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/lyrics/feelin/presentation/view/mypage/userinfo/UserInfoScreen.kt` around lines 41 - 44, Replace the unnecessary remembered mutable state used for static mock values with plain immutable vals: change loginInfoItem, uid, gender, and birthYear (currently created with remember { mutableStateOf(...) }) to simple val declarations (e.g., val loginInfoItem = LoginInfoConfigs.kakao) since these are static design placeholders; when real data arrives, hoist them to a ViewModel and expose as state instead.
74-74: TODO noted for UID copy functionality.The clipboard copy and snackbar feedback will need to be implemented. Consider using
ClipboardManagerfromLocalClipboardManagerand aSnackbarHostStatehoisted to the Scaffold.Would you like me to generate the implementation for the UID copy functionality?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/lyrics/feelin/presentation/view/mypage/userinfo/UserInfoScreen.kt` at line 74, The TODO in SettingInfoActionButton should be replaced with code that copies the UID to the clipboard and shows a snackbar: obtain LocalClipboardManager.current (e.g., clipboardManager) and a rememberCoroutineScope plus the Scaffold's SnackbarHostState (or create one if not already hoisted), then in SettingInfoActionButton's onClick call clipboardManager.setText(AnnotatedString(uid)) and launch a coroutine to call snackbarHostState.showSnackbar("복사되었습니다" or similar); update UserInfoScreen to accept or provide the SnackbarHostState and the UID string used for copying so the button can access both.app/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt (2)
69-73: Logout and withdrawal actions need implementation.Currently using
SettingInfoItemwhich doesn't support click actions. When implementing the actual logout/withdrawal flows, these will need to be replaced with clickable components orSettingInfoItemwill need an optionalonClickparameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt` around lines 69 - 73, SettingInfoItem instances for "로그아웃" and "회원 탈퇴" are not clickable and therefore lack the required logout/withdrawal behavior; update the UI so these actions are tappable by either adding an optional onClick parameter to SettingInfoItem (and wiring it through to its clickable container) or replacing those two items in SettingScreen with a clickable composable (e.g., ClickableSettingItem) that calls your logout and withdrawal handlers; ensure you reference SettingInfoItem in SettingScreen and hook the appropriate logout/withdrawal functions so the UI triggers the flows.
77-81: Consider responsive sizing for the feedback banner.Fixed dimensions (350.dp × 110.dp) may not render well on all screen widths.
Proposed improvement for responsive width
Image( painter = painterResource(R.drawable.feedback_banner), contentDescription = "feedback banner", - modifier = Modifier.width(350.dp).height(110.dp) + modifier = Modifier + .fillMaxWidth() + .padding(horizontal = 20.dp) + .height(110.dp), + contentScale = ContentScale.FillWidth )You'll need to import
ContentScalefromandroidx.compose.ui.layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt` around lines 77 - 81, The Image currently uses fixed dimensions which break on different screens; update the Image composable (the call using painterResource(R.drawable.feedback_banner)) to use a responsive Modifier such as fillMaxWidth() combined with either a fixed height or aspectRatio, and set contentScale = ContentScale.Crop (import androidx.compose.ui.layout.ContentScale) so the banner scales and crops correctly across devices; ensure the modifier replaces width(350.dp).height(110.dp) with the responsive combination and keep contentDescription unchanged.app/src/main/java/com/lyrics/feelin/presentation/view/mypage/component/SettingInfoItem.kt (1)
83-89: Consider reordering modifiers for clearer intent.The current order
.clip().background().clickable().padding()works, but placing.clickable()before.background()is more idiomatic in Compose as it ensures the ripple indication is drawn under the background. The current order works because.clip()is applied first, but it's a subtle distinction.This is minor and doesn't affect functionality.
🔧 Optional modifier reorder
Box( modifier = modifier .height(28.dp) .clip(shape = RoundedCornerShape(4.dp)) - .background(color = feelinColors.brandSecondary) .clickable(onClick = onClick) + .background(color = feelinColors.brandSecondary) .padding(horizontal = 8.dp, vertical = 4.dp), contentAlignment = Alignment.Center, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/lyrics/feelin/presentation/view/mypage/component/SettingInfoItem.kt` around lines 83 - 89, Reorder the Box modifier chain in SettingInfoItem (the Box using modifier.height(...).clip(...).background(...).clickable(...).padding(...)) so that .clickable(onClick = onClick) appears before .background(color = feelinColors.brandSecondary); keep .clip(RoundedCornerShape(4.dp)) directly after height to preserve rounded bounds and retain .padding(horizontal = 8.dp, vertical = 4.dp) at the end so the clickable ripple is drawn under the background while behavior and layout remain unchanged.app/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.kt (1)
222-222: Consider deriving bottom bar selection from navigation state.The
selectedBottomBarIndexis local state initialized fromselectedIndexparameter. While this works for the current flow, it creates a duplication of truth (local state vs. navigation state). A more robust approach would derive the selection fromnavController.currentBackStackEntryAsState().However, given the current nested graph structure and how
selectedIndexis passed correctly to each screen'sMainScaffold, this works correctly for typical user flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.kt` at line 222, The local mutable state selectedBottomBarIndex (created with remember { mutableIntStateOf(selectedIndex) }) duplicates navigation truth; replace it by deriving the selected index from navController.currentBackStackEntryAsState(), mapping the active destination route to the bottom-bar index (e.g., via when or a route->index helper) so selection reflects navigation changes automatically; remove the mutable variable and any setters and use the derived val wherever selectedBottomBarIndex was used (reference symbols: selectedBottomBarIndex, selectedIndex, navController.currentBackStackEntryAsState()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.kt`:
- Line 222: The local mutable state selectedBottomBarIndex (created with
remember { mutableIntStateOf(selectedIndex) }) duplicates navigation truth;
replace it by deriving the selected index from
navController.currentBackStackEntryAsState(), mapping the active destination
route to the bottom-bar index (e.g., via when or a route->index helper) so
selection reflects navigation changes automatically; remove the mutable variable
and any setters and use the derived val wherever selectedBottomBarIndex was used
(reference symbols: selectedBottomBarIndex, selectedIndex,
navController.currentBackStackEntryAsState()).
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/mypage/component/SettingInfoItem.kt`:
- Around line 83-89: Reorder the Box modifier chain in SettingInfoItem (the Box
using
modifier.height(...).clip(...).background(...).clickable(...).padding(...)) so
that .clickable(onClick = onClick) appears before .background(color =
feelinColors.brandSecondary); keep .clip(RoundedCornerShape(4.dp)) directly
after height to preserve rounded bounds and retain .padding(horizontal = 8.dp,
vertical = 4.dp) at the end so the clickable ripple is drawn under the
background while behavior and layout remain unchanged.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt`:
- Around line 69-73: SettingInfoItem instances for "로그아웃" and "회원 탈퇴" are not
clickable and therefore lack the required logout/withdrawal behavior; update the
UI so these actions are tappable by either adding an optional onClick parameter
to SettingInfoItem (and wiring it through to its clickable container) or
replacing those two items in SettingScreen with a clickable composable (e.g.,
ClickableSettingItem) that calls your logout and withdrawal handlers; ensure you
reference SettingInfoItem in SettingScreen and hook the appropriate
logout/withdrawal functions so the UI triggers the flows.
- Around line 77-81: The Image currently uses fixed dimensions which break on
different screens; update the Image composable (the call using
painterResource(R.drawable.feedback_banner)) to use a responsive Modifier such
as fillMaxWidth() combined with either a fixed height or aspectRatio, and set
contentScale = ContentScale.Crop (import
androidx.compose.ui.layout.ContentScale) so the banner scales and crops
correctly across devices; ensure the modifier replaces
width(350.dp).height(110.dp) with the responsive combination and keep
contentDescription unchanged.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/mypage/userinfo/UserInfoScreen.kt`:
- Around line 41-44: Replace the unnecessary remembered mutable state used for
static mock values with plain immutable vals: change loginInfoItem, uid, gender,
and birthYear (currently created with remember { mutableStateOf(...) }) to
simple val declarations (e.g., val loginInfoItem = LoginInfoConfigs.kakao) since
these are static design placeholders; when real data arrives, hoist them to a
ViewModel and expose as state instead.
- Line 74: The TODO in SettingInfoActionButton should be replaced with code that
copies the UID to the clipboard and shows a snackbar: obtain
LocalClipboardManager.current (e.g., clipboardManager) and a
rememberCoroutineScope plus the Scaffold's SnackbarHostState (or create one if
not already hoisted), then in SettingInfoActionButton's onClick call
clipboardManager.setText(AnnotatedString(uid)) and launch a coroutine to call
snackbarHostState.showSnackbar("복사되었습니다" or similar); update UserInfoScreen to
accept or provide the SnackbarHostState and the UID string used for copying so
the button can access both.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f5ba8f38-43f2-413c-bc00-87f96d9e64f2
⛔ Files ignored due to path filters (1)
app/src/main/res/drawable/feedback_banner.pngis excluded by!**/*.png
📒 Files selected for processing (11)
app/src/main/java/com/lyrics/feelin/navigation/FeelinDestination.ktapp/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.ktapp/src/main/java/com/lyrics/feelin/presentation/designsystem/theme/Color.ktapp/src/main/java/com/lyrics/feelin/presentation/view/login/SocialLoginButton.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/MyPageScreen.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/component/MyPageItemRowShell.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/component/SettingInfoItem.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/component/SettingMenuItem.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/component/UserLoginInfoItem.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/userinfo/UserInfoScreen.kt
Member
Author
|
Potential issue 미발견으로 #14 병합 이전 혹은 직후 바로 병합하겠습니다. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
하단 내비게이션 바를 이용해 진입하는 화면이 nested route로 변경됩니다.
ScreenShots (If needed)
Other information:
기존 코드에 있던 로컬에 저장하는 더미 유저 초기화 기능은 연결하지 않았습니다. (빠르게 실제 기능을 연동해야 합니다.)
Summary by CodeRabbit
New Features