Conversation
WalkthroughAdds three new Compose UI components (HsLinkSelectButton, HsLinkTextField, HsLinkTopBar), three drawable icons, updates Modifier.noRippleClickable signature to accept an enabled flag, updates a bottom bar call site, and removes an IDE configuration file. No exported API removals beyond the modified modifier signature. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Compose UI (Button)
participant Mod as noRippleClickable
participant OS as Clickable (framework)
participant App as onClick handler
User->>UI: Tap button
UI->>Mod: invoke noRippleClickable(enabled)
Mod->>OS: forward to clickable(enabled)
alt enabled == true
OS->>App: emit onClick
App->>App: execute action
else enabled == false
OS-->>User: ignore tap
end
note right of Mod #f7f3c6: Modifier signature updated\n(enabled: Boolean = true)
sequenceDiagram
autonumber
actor User
participant TextField as HsLinkTextField
participant Focus as FocusRequester/IME
participant App as value/onDone
User->>TextField: Type characters / press IME Done
TextField->>App: onValueChanged(newValue)
TextField->>Focus: requestFocus / onFocusChanged
alt IME Done pressed
TextField->>App: onDoneAction()
end
note right of TextField #e6f7ff: Supports placeholder, leading icon,\nand password visibility toggle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 2
🧹 Nitpick comments (6)
app/src/main/res/drawable/ic_topbar_plus.xml (1)
6-8: Make the icon themeable and align to standard sizing.
- Hardcoded fillColor (#3B3C3F) won’t adapt to dark mode; prefer tinting in Compose (Icon tint) or reference a color resource.
- 48dp viewport/size is atypical for app bar actions; consider 24dp to match Material metrics.
app/src/main/java/com/hsLink/hslink/presentation/main/component/MainBottomBar.kt (1)
87-103: Use selectable semantics for tabs; avoid double announcements.
- Replace noRippleClickable with Modifier.selectable(selected, onClick, role = Role.Tab, enabled = !selected) for correct a11y.
- Set Icon contentDescription = null to prevent reading the same label twice; the Text already conveys it.
Apply this diff:
@@ - Column( - modifier = modifier - .noRippleClickable(true, onClick), + Column( + modifier = modifier + .selectable( + selected = selected, + onClick = onClick, + enabled = !selected, + role = Role.Tab + ), horizontalAlignment = Alignment.CenterHorizontally, verticalArrangement = Arrangement.spacedBy(4.dp), ) { Icon( imageVector = ImageVector.vectorResource(iconRes), - contentDescription = stringResource(tab.contentDescription), + contentDescription = null, tint = Color.Unspecified, modifier = Modifier.size(24.dp), )Add imports if missing:
import androidx.compose.foundation.selection.selectable import androidx.compose.ui.semantics.Roleapp/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTopBar.kt (1)
22-35: Expose click handlers and tighten layout defaults.
- Consider onLeftClick/onRightFirstClick/onRightSecondClick lambdas and apply clickable/noRippleClickable for UX.
- Increase horizontal padding to 16.dp to align with Material ergonomics.
Example signature tweak:
-fun HsLinkTopBar( +fun HsLinkTopBar( modifier: Modifier = Modifier, title: @Composable () -> Unit = {}, @DrawableRes rightIconFirst: Int? = null, @DrawableRes rightIconSecond: Int? = null, - @DrawableRes leftIcon: Int? = null, + @DrawableRes leftIcon: Int? = null, + onLeftClick: (() -> Unit)? = null, + onRightFirstClick: (() -> Unit)? = null, + onRightSecondClick: (() -> Unit)? = null, ) { @@ - .padding(horizontal = 4.dp, vertical = 8.dp), + .padding(horizontal = 16.dp, vertical = 8.dp),Then wrap each Icon with a modifier applying click when its handler is non-null.
app/src/main/java/com/hsLink/hslink/core/util/Modifier.kt (1)
9-19: Current implementation is correct; proposed a11y enhancement has breaking change.The code change is solid. The optional a11y enhancement (adding
roleandonClickLabel) is beneficial but introduces a breaking change: the positional argument call atMainBottomBar.kt:89(.noRippleClickable(true, onClick)) would fail because the new parameters shift whereonClickbinds. The named-argument call atHsLinkSelectButton.kt:78-80would work fine with defaults.If implementing the enhancement, you must refactor
MainBottomBar.kt:89to use named arguments:.noRippleClickable(enabled = true, onClick = onClick)app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTextField.kt (1)
73-76: Consider using the complete TextStyle from the theme.Currently only the
fontStyleproperty is extracted fromHsLinkTheme.typography.body_16Normal, which means other properties likefontSize,fontWeight, andletterSpacingare not applied. Consider using:-textStyle = TextStyle( - color = HsLinkTheme.colors.Grey700, - fontStyle = HsLinkTheme.typography.body_16Normal.fontStyle -) +textStyle = HsLinkTheme.typography.body_16Normal.copy( + color = HsLinkTheme.colors.Grey700 +)app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkSelectButton.kt (1)
49-53: Simplify redundant borderColor logic.Both the
isSelectedandelsebranches returnSkyBlue500, making the distinction unnecessary.Apply this diff:
val borderColor = when { !isEnabled -> HsLinkTheme.colors.Grey200 - isSelected -> HsLinkTheme.colors.SkyBlue500 - else -> HsLinkTheme.colors.SkyBlue500 + else -> HsLinkTheme.colors.SkyBlue500 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.idea/deploymentTargetSelector.xml(0 hunks)app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkSelectButton.kt(1 hunks)app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTextField.kt(1 hunks)app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTopBar.kt(1 hunks)app/src/main/java/com/hsLink/hslink/core/util/Modifier.kt(1 hunks)app/src/main/java/com/hsLink/hslink/presentation/main/component/MainBottomBar.kt(1 hunks)app/src/main/res/drawable/ic_textfield_edit.xml(1 hunks)app/src/main/res/drawable/ic_textfield_plus.xml(1 hunks)app/src/main/res/drawable/ic_topbar_plus.xml(1 hunks)
💤 Files with no reviewable changes (1)
- .idea/deploymentTargetSelector.xml
🧰 Additional context used
🧬 Code graph analysis (3)
app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTextField.kt (1)
app/src/main/java/com/hsLink/hslink/core/designsystem/theme/Theme.kt (1)
HsLinkTheme(37-57)
app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTopBar.kt (1)
app/src/main/java/com/hsLink/hslink/core/designsystem/theme/Theme.kt (1)
HsLinkTheme(37-57)
app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkSelectButton.kt (1)
app/src/main/java/com/hsLink/hslink/core/designsystem/theme/Theme.kt (1)
HsLinkTheme(37-57)
🔇 Additional comments (5)
app/src/main/res/drawable/ic_textfield_plus.xml (1)
1-9: LGTM! Icon drawable follows Android conventions and is properly structured.The vector drawable is correctly defined with consistent dimensions, a well-formed path describing the plus icon, and appropriate styling. The naming convention (
ic_textfield_plus.xml) aligns with Android drawable resource standards. This icon integrates well with the HsTextField component being introduced in this PR.app/src/main/res/drawable/ic_textfield_edit.xml (1)
1-12: Name-glyph mismatch and theming concerns verified; limited to preview usage.The drawable is used only in a preview demo (HsLinkTextField.kt:118). All concerns in the review are valid:
- Name mismatch: Icon is an "X" (clear/close) but named ic_textfield_edit—rename to ic_textfield_clear for clarity.
- Hardcoded colors: Rendered with
tint = Color.Unspecified, which uses baked-in SVG colors (#D1D2D3, #FFFFFF). Won't adapt to dark mode.- Non-standard size: 21dp deviates from Material icon grid (24dp standard).
Since this is preview-only with no production usages found, verify with the developer whether this is intentional or if the drawable requires updating for potential future use.
app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkSelectButton.kt (3)
23-36: LGTM!The enum design with size variants and the composable
getTextStyle()method provides a clean API for button sizing. The consistent horizontal padding across sizes is a good design choice.
67-94: LGTM!The button implementation correctly applies the design system tokens, handles the enabled/selected states properly, and uses the
noRippleClickablemodifier to respect theisEnabledparameter.
96-185: Excellent preview coverage!The preview comprehensively demonstrates all size variants (Small, Medium, Large) in all states (enabled/unselected, enabled/selected, disabled). This is very helpful for design system documentation and development.
| Icon( | ||
| imageVector = ImageVector.vectorResource(id = leadingIconRes ?: 0), | ||
| contentDescription = null, | ||
| tint = Color.Unspecified | ||
| ) |
There was a problem hiding this comment.
Fix crash when leadingIconRes is null.
The Icon is always rendered even when leadingIconRes is null, defaulting to resource ID 0, which will cause a crash. Conditionally render the icon only when a valid resource is provided.
Apply this diff:
- Icon(
- imageVector = ImageVector.vectorResource(id = leadingIconRes ?: 0),
- contentDescription = null,
- tint = Color.Unspecified
- )
+ leadingIconRes?.let { iconRes ->
+ Icon(
+ imageVector = ImageVector.vectorResource(id = iconRes),
+ contentDescription = null,
+ tint = Color.Unspecified
+ )
+ }🤖 Prompt for AI Agents
In
app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTextField.kt
around lines 96 to 100, the Icon is always created using
ImageVector.vectorResource(id = leadingIconRes ?: 0) which passes 0 when
leadingIconRes is null and crashes; change this to render the Icon only when
leadingIconRes is non-null — check if leadingIconRes != null and only then call
ImageVector.vectorResource and emit the Icon composable so null-safe rendering
is ensured.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of the HsLinkTextField.kt file to see the Icon implementation
cat app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTextField.kt | sed -n '90,105p'
Length of output: 503
@vvan2, 현재 코드에서는 leadingIconRes가 null일 때 문제가 발생합니다:
Icon(
imageVector = ImageVector.vectorResource(id = leadingIconRes ?: 0),
contentDescription = null,
tint = Color.Unspecified
)문제점:
leadingIconRes가null이면0으로 대체되어ImageVector.vectorResource(id = 0)이 호출됩니다- 리소스 ID
0은 존재하지 않기 때문에Resources.NotFoundException이 발생하여 앱이 크래시됩니다 - 현재는 Icon이 항상 렌더링되므로, leadingIconRes를 전달하지 않으면 무조건 크래시가 발생합니다
해결방법:
아이콘이 제공될 때만 렌더링하도록 조건부로 처리해야 합니다:
leadingIconRes?.let { iconRes ->
Icon(
imageVector = ImageVector.vectorResource(id = iconRes),
contentDescription = null,
tint = Color.Unspecified
)
}이렇게 하면 leadingIconRes가 null일 때는 아이콘이 렌더링되지 않고, 값이 있을 때만 안전하게 렌더링됩니다. 🐰
app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTopBar.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTopBar.kt (1)
36-59: Consider adding click handlers for icon interactivity.Top bar icons typically need to respond to user taps (e.g., navigation back, opening menus). Consider adding optional
onClicklambda parameters for each icon so the component supports interaction out of the box.Example addition:
@Composable fun HsLinkTopBar( modifier: Modifier = Modifier, title: @Composable () -> Unit = {}, @DrawableRes leftIcon: Int? = null, onLeftIconClick: (() -> Unit)? = null, @DrawableRes rightIconFirst: Int? = null, onRightIconFirstClick: (() -> Unit)? = null, @DrawableRes rightIconSecond: Int? = null, onRightIconSecondClick: (() -> Unit)? = null, ) { // ... in Row: leftIcon?.let { Icon( imageVector = ImageVector.vectorResource(id = it), contentDescription = "...", modifier = Modifier.clickable( enabled = onLeftIconClick != null, onClick = { onLeftIconClick?.invoke() } ) ) } // ... similar for other icons }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTopBar.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTopBar.kt (1)
app/src/main/java/com/hsLink/hslink/core/designsystem/theme/Theme.kt (1)
HsLinkTheme(37-57)
🔇 Additional comments (1)
app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTopBar.kt (1)
36-59: Previous crash risk resolved.The
?.letpattern now ensures icons are only rendered when non-null, fixing the critical issue flagged in the previous review.
| Icon( | ||
| imageVector = ImageVector.vectorResource(id = it), | ||
| contentDescription = "leftIcon" | ||
| ) | ||
| } | ||
|
|
||
| title() | ||
|
|
||
| Spacer(modifier = Modifier.weight(1f)) | ||
|
|
||
| rightIconFirst?.let { | ||
| Icon( | ||
| imageVector = ImageVector.vectorResource(id = it), | ||
| contentDescription = "rightIconFirst" | ||
| ) | ||
| } | ||
|
|
||
| rightIconSecond?.let { | ||
| Icon( | ||
| imageVector = ImageVector.vectorResource(id = it), | ||
| contentDescription = "rightIconSecond" | ||
| ) |
There was a problem hiding this comment.
Improve contentDescription for accessibility.
Generic strings like "leftIcon" don't help screen reader users understand what each icon does. Consider:
- Pass
contentDescriptionas parameters (e.g.,leftIconContentDescription: String?) so callers can provide meaningful descriptions like "Navigate back" or "Add item" - Use
contentDescription = nullonly if icons are purely decorative
Apply this diff to add content description parameters:
@Composable
fun HsLinkTopBar(
modifier: Modifier = Modifier,
title: @Composable () -> Unit = {},
@DrawableRes rightIconFirst: Int? = null,
+ rightIconFirstContentDescription: String? = null,
@DrawableRes rightIconSecond: Int? = null,
+ rightIconSecondContentDescription: String? = null,
@DrawableRes leftIcon: Int? = null,
+ leftIconContentDescription: String? = null,
) {
Row(
modifier = modifier
.fillMaxWidth()
.padding(horizontal = 4.dp, vertical = 8.dp),
verticalAlignment = Alignment.CenterVertically,
horizontalArrangement = Arrangement.spacedBy(4.dp)
) {
leftIcon?.let {
Icon(
imageVector = ImageVector.vectorResource(id = it),
- contentDescription = "leftIcon"
+ contentDescription = leftIconContentDescription
)
}
title()
Spacer(modifier = Modifier.weight(1f))
rightIconFirst?.let {
Icon(
imageVector = ImageVector.vectorResource(id = it),
- contentDescription = "rightIconFirst"
+ contentDescription = rightIconFirstContentDescription
)
}
rightIconSecond?.let {
Icon(
imageVector = ImageVector.vectorResource(id = it),
- contentDescription = "rightIconSecond"
+ contentDescription = rightIconSecondContentDescription
)
}
}
}🤖 Prompt for AI Agents
In
app/src/main/java/com/hsLink/hslink/core/designsystem/component/HsLinkTopBar.kt
around lines 37 to 58, the Icon contentDescription values are hard-coded to
generic strings; update the function signature to accept nullable
contentDescription parameters for each icon (e.g., leftIconContentDescription:
String? = null, rightIconFirstContentDescription: String? = null,
rightIconSecondContentDescription: String? = null), replace the hard-coded
contentDescription values with those parameters when invoking Icon, and ensure
callers pass meaningful descriptions (or pass null for purely decorative icons).
1 participant
ISSUE
❗ WORK DESCRIPTION
유효성 검사로직이나 추가로 컴포넌트 붙는 건 따로 작업해주세요!
Preview에 전부 넣어놨으니 확인하면서 작업해주시면 됩니다.
📸 SCREENSHOT
📢 TO REVIEWERS
Summary by CodeRabbit
New Features
Chores
Refactor