[FE-Feat] Input 컴포넌트의 border 적용 위치를 동적으로 선택 가능하도록 구현 #100
Conversation
`borderPlacement = 'container' | 'inputField'`
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request updates the Input component suite by modifying several import paths to reflect a new directory structure, introducing a new property ( Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
frontend/src/components/Input/Core/inputField.css.ts (2)
10-10: LGTM! Good improvements to design system compliance.The changes properly utilize theme variables and ensure consistent styling. The addition of
boxSizing: 'border-box'is particularly important for predictable layouts.Consider adding a comment explaining why
height: '100%'is necessary, as this might not be immediately obvious to other developers:base: { flex: 1, + // Ensure the input field container fills its parent's height height: '100%', display: 'flex',Also applies to: 14-16, 18-18
7-26: LGTM! Implementation aligns well with PR objectives.The removal of direct border styling and the maintenance of only
borderWidthproperly supports the newborderPlacementfeature while preserving design system consistency.This is a good example of separation of concerns - the base styles remain in this file while the dynamic border styling is handled by the consumer components based on the
borderPlacementprop.frontend/src/components/Input/MultiInput.tsx (2)
35-37: Use React.Children.toArray for consistent child handlingWhen working with
children, it's recommended to useReact.Children.toArray(children)instead ofArray.from(children as ReactNode[]). This ensures that all types ofchildren(single elements, arrays, strings, etc.) are handled correctly and each child is assigned a unique key.Apply this change:
const childElements = children ? - Array.from(children as ReactNode[]).filter(isValidElement) + React.Children.toArray(children).filter(isValidElement) : [];
75-75: Use React.cloneElement to clone and modify React elementsDirectly accessing
separator.typeandseparator.propscan lead to issues, especially with more complex elements. UsingReact.cloneElementis the standard way to clone and modify React elements while preserving refs and keys.Apply this change:
return ( - <separator.type {...separator.props} width={ICON_WIDTH} /> + React.cloneElement(separator, { width: ICON_WIDTH }) );frontend/src/utils/jsxUtils.tsx (1)
8-15: Use React.cloneElement and assign unique keys to elementsWhen manipulating React elements, it's best to use
React.cloneElementto clone and modify elements. Additionally, ensure that each element in the array has a uniquekeyprop to prevent React warnings about missing keys.Apply these changes:
+import React, { cloneElement } from 'react'; export const intersperseElement = ( childElements: ReactNode[], separator?: JSX.Element, ): ReactNode[] => { const result: ReactNode[] = []; childElements.forEach((child, index) => { - result.push(child); + result.push( + React.isValidElement(child) + ? React.cloneElement(child, { key: `child-${index}` }) + : child + ); if (separator && index < childElements.length - 1) { - result.push(<separator.type {...separator.props} key={index} />); + result.push(React.cloneElement(separator, { key: `separator-${index}` })); } }); return result; };This ensures that all elements have unique keys and uses
React.cloneElementfor safe cloning.frontend/src/components/Input/index.css.ts (2)
31-32: Remove redundant border width definition.The border width is defined twice: once in the
bordershorthand property and again inborderWidth. This is redundant and could lead to inconsistencies.base: { - border: `1px solid ${vars.color.Ref.Netural[300]}`, - borderWidth: 1.5, + border: `1.5px solid ${vars.color.Ref.Netural[300]}`, },
15-18: Consider using theme variables for fixed dimensions.The fixed height of 40px should ideally come from the theme variables for consistency across components.
Consider defining a height variable in your theme and using it here:
- height: 40, + height: vars.spacing.inputHeight, // Add this to your themefrontend/src/components/Input/SingleInput.tsx (1)
28-28: Simplify className concatenation.The template literal string could be simplified using the
clsxorclassnameslibrary for better readability and maintainability.Consider using a utility library:
- <div className={`${inputFieldsContainerStyle} ${interactableBorderStyle({ isValid })}`}> + <div className={clsx(inputFieldsContainerStyle, interactableBorderStyle({ isValid }))}>frontend/src/components/Input/Core/InputField.tsx (1)
30-35: Improve className handling and type safety.The current implementation has a few areas for improvement:
- Complex template literal for className
- No type safety for borderPlacement value
Consider these improvements:
+ import clsx from 'clsx'; <div - className={` - ${inputFieldContainerStyle({ type })} - ${borderPlacement === 'inputField' && interactableBorderStyle({ isValid })} - `} + className={clsx( + inputFieldContainerStyle({ type }), + borderPlacement === 'inputField' && interactableBorderStyle({ isValid }) + )} onClick={handleContainerClick} >frontend/src/components/Input/Input.stories.tsx (2)
8-38: Document the new borderPlacement prop in argTypes.The new borderPlacement prop should be documented in the Storybook configuration.
Add the prop documentation:
argTypes: { + borderPlacement: { + control: 'radio', + options: ['container', 'inputField'], + description: '테두리 적용 위치', + }, isValid: { control: 'boolean', description: '입력 필드 유효성', },
67-68: Add a story demonstrating default borderPlacement behavior.The stories show both borderPlacement options but don't demonstrate what happens when the prop is not provided.
Consider adding a story that omits the borderPlacement prop to show the default behavior.
Also applies to: 98-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
frontend/src/components/Input/Core/HelperText.tsx(1 hunks)frontend/src/components/Input/Core/InputField.tsx(3 hunks)frontend/src/components/Input/Core/Label.tsx(1 hunks)frontend/src/components/Input/Core/helperText.css.ts(1 hunks)frontend/src/components/Input/Core/inputField.css.ts(1 hunks)frontend/src/components/Input/Core/label.css.ts(1 hunks)frontend/src/components/Input/Input.stories.tsx(3 hunks)frontend/src/components/Input/InputContext.ts(1 hunks)frontend/src/components/Input/MultiInput.tsx(3 hunks)frontend/src/components/Input/SingleInput.tsx(2 hunks)frontend/src/components/Input/index.css.ts(3 hunks)frontend/src/utils/jsxUtils.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- frontend/src/components/Input/Core/label.css.ts
- frontend/src/components/Input/Core/helperText.css.ts
- frontend/src/components/Input/Core/HelperText.tsx
- frontend/src/components/Input/Core/Label.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Send PR Notification to Reviewers
🔇 Additional comments (3)
frontend/src/components/Input/Core/inputField.css.ts (2)
4-5: LGTM! Good use of path aliases.Using
@alias instead of relative paths improves maintainability and makes the imports more resilient to refactoring.
21-24: LGTM! Clear and appropriate cursor variants.The variants properly differentiate between text and select input types through cursor styles.
frontend/src/components/Input/InputContext.ts (1)
8-8: AddingborderPlacementto InputContextPropsThe addition of
borderPlacementas an optional property inInputContextPropsenhances the context to support dynamic border placement. This allows components consuming the context to adjust styling based on the border placement.
| <div className={` | ||
| ${inputFieldsContainerStyle} | ||
| ${borderPlacement === 'container' && interactableBorderStyle({ isValid })} | ||
| `} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid including 'false' in class names when using conditional expressions
In the template literal for className, if borderPlacement is not 'container', the expression ${borderPlacement === 'container' && interactableBorderStyle({ isValid })} will return false, which will be included in the string. This can lead to unintended class names like "someClass false".
Consider using an array of class names and filtering out falsy values:
-<div className={`
- ${inputFieldsContainerStyle}
- ${borderPlacement === 'container' && interactableBorderStyle({ isValid })}
- `}
->
+<div
+ className={[
+ inputFieldsContainerStyle,
+ borderPlacement === 'container' && interactableBorderStyle({ isValid }),
+ ]
+ .filter(Boolean)
+ .join(' ')}
+>Alternatively, you can use a utility library like classnames for cleaner class name management.
📝 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.
| <div className={` | |
| ${inputFieldsContainerStyle} | |
| ${borderPlacement === 'container' && interactableBorderStyle({ isValid })} | |
| `} | |
| > | |
| <div | |
| className={[ | |
| inputFieldsContainerStyle, | |
| borderPlacement === 'container' && interactableBorderStyle({ isValid }), | |
| ] | |
| .filter(Boolean) | |
| .join(' ')} | |
| > |
| export interface MultiInputProps extends CommonInputProps, PropsWithChildren { | ||
| separator?: string | JSX.Element; | ||
| borderPlacement: 'container' | 'inputField'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adding a required prop may cause breaking changes
The borderPlacement prop has been added as a required field in the MultiInputProps interface without a default value. This could introduce breaking changes in existing code that uses MultiInput without specifying borderPlacement. Consider providing a default value to maintain backward compatibility.
You can set a default value when destructuring props:
export const MultiInput = ({
label,
type = 'text',
isValid = true,
required = false,
separator = '',
hint,
error,
- borderPlacement,
+ borderPlacement = 'inputField',
children,
}: MultiInputProps) => {This way, existing usages will default to 'inputField' if borderPlacement is not specified.
📝 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.
| export interface MultiInputProps extends CommonInputProps, PropsWithChildren { | |
| separator?: string | JSX.Element; | |
| borderPlacement: 'container' | 'inputField'; | |
| } | |
| export interface MultiInputProps extends CommonInputProps, PropsWithChildren { | |
| separator?: string | JSX.Element; | |
| borderPlacement: 'container' | 'inputField'; | |
| } | |
| export const MultiInput = ({ | |
| label, | |
| type = 'text', | |
| isValid = true, | |
| required = false, | |
| separator = '', | |
| hint, | |
| error, | |
| borderPlacement = 'inputField', | |
| children, | |
| }: MultiInputProps) => { | |
| // Component implementation | |
| } |
There was a problem hiding this comment.
요 리뷰도 확인해주세요~!
borderPlacement도 기본값 설정을 해주고, 옵셔널로 받아도 될 것 같아요.
There was a problem hiding this comment.
@hamo-o I agree with your suggestion. Here's how we can make borderPlacement optional with a default value:
export interface MultiInputProps extends CommonInputProps, PropsWithChildren {
separator?: string | JSX.Element;
borderPlacement?: 'container' | 'inputField';
}
export const MultiInput = ({
label,
type = 'text',
isValid = true,
required = false,
separator = '',
hint,
error,
borderPlacement = 'inputField',
children,
}: MultiInputProps) => {
// ... rest of the implementation
}✏️ Learnings added
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#100
File: frontend/src/components/Input/MultiInput.tsx:19-22
Timestamp: 2025-02-06T02:51:01.154Z
Learning: In the Input component, the `borderPlacement` prop should be optional with a default value of 'inputField' to maintain backward compatibility and improve developer experience.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| import type { ReactNode } from '@tanstack/react-router'; | ||
| import type { JSX } from 'react'; |
There was a problem hiding this comment.
Incorrect import source for ReactNode
The ReactNode type should be imported from 'react', not '@tanstack/react-router'. Importing from the incorrect source may lead to unexpected type issues.
Apply this fix:
-import type { ReactNode } from '@tanstack/react-router';
+import type { ReactNode } from 'react';📝 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.
| import type { ReactNode } from '@tanstack/react-router'; | |
| import type { JSX } from 'react'; | |
| import type { ReactNode } from 'react'; | |
| import type { JSX } from 'react'; |
There was a problem hiding this comment.
🧩 Analysis chain
@hamo-o 네, 제가 드린 리뷰가 정확합니다. ReactNode 타입은 React의 핵심 타입으로, 'react' 패키지에서 직접 import하는 것이 best practice입니다. 제안드린 수정사항을 적용해주시면 감사하겠습니다.
(_/）
(•ω• )
ヽ( 、ヽ)
🌐 Web query:
What is the correct import source for ReactNode type in TypeScript? Is it available in @tanstack/react-router?
💡 Result:
In TypeScript, the `ReactNode` type should be imported directly from the `react` package, not from `@tanstack/react-router`. According to the search results:
1. **ReactNode Definition**:
- `ReactNode` represents any renderable React content (elements, strings, numbers, etc.) and is part of React's core type system[1][7][10].
- Example import:
```tsx
import { ReactNode } from 'react';
```
2. **@tanstack/react-router Context**:
- TanStack Router focuses on routing-specific types like `Route`, `LinkProps`, and router configuration utilities[2][5][8].
- It does **not** export `ReactNode` - this type remains exclusively part of React's type definitions[1][4][7].
For TypeScript projects using both libraries:
```tsx
// Correct usage:
import { ReactNode } from 'react';
import { Link } from '@tanstack/react-router';
function Component({ children }: { children: ReactNode }) {
return <Link to="/">{children}</Link>;
}
```
The error "Type Element is Not Assignable to Type Reactnode" typically occurs when mixing incompatible type declarations[4], which can be resolved by ensuring proper imports from `react` rather than routing libraries[1][7].
Citations:
- 1: https://dev.to/elhamnajeebullah/react-typescript-what-is-reactnode-and-when-to-use-it-3660
- 2: https://tanstack.com/router/v1/docs/framework/react/guide/type-safety
- 3: https://tanstack.com/start/latest/docs/framework/react/learn-the-basics
- 4: https://www.dhiwise.com/post/resolve-type-element-is-not-assignable-to-type-reactnode
- 5: https://www.npmjs.com/package/@tanstack/react-router
- 6: https://dev.to/franklin030601/managing-state-with-react-query-1842
- 7: https://www.totaltypescript.com/jsx-element-vs-react-reactnode
- 8: https://tanstack.com/router/latest/docs/framework/react/overview
- 9: https://github.com/TanStack/router/issues/1271
- 10: https://react.dev/learn/typescript
@hamo-o 네, 제가 드린 리뷰가 정확합니다. 좀 더 자세히 설명드리면:
ReactNode는 React의 핵심 타입으로, 'react' 패키지에서 직접 import 해야 합니다.@tanstack/react-router는 라우팅 관련 타입들만 제공하며,ReactNode타입은 제공하지 않습니다.- 잘못된 패키지에서 타입을 import하면 타입 호환성 문제가 발생할 수 있습니다.
제안드린 수정사항을 적용해주시면 감사하겠습니다.
(_/）
(•ω• )
ヽ( 、ヽ)
✏️ Learnings added
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#100
File: frontend/src/utils/jsxUtils.tsx:1-2
Timestamp: 2025-02-06T02:50:41.482Z
Learning: ReactNode type should be imported directly from 'react' package, not from third-party packages that might re-export it, to follow best practices and reduce potential version conflicts.
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#100
File: frontend/src/utils/jsxUtils.tsx:1-2
Timestamp: 2025-02-06T02:50:41.482Z
Learning: @tanstack/react-router package focuses on routing-specific types and does not export ReactNode type. Always import ReactNode from 'react' package to avoid type compatibility issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| import type { ReactNode } from '@tanstack/react-router'; | ||
| import type { JSX } from 'react'; |
| <div className={` | ||
| ${inputFieldsContainerStyle} | ||
| ${borderPlacement === 'container' && interactableBorderStyle({ isValid })} | ||
| `} |
There was a problem hiding this comment.
p3;
클래스 이름 여러 개 사용할 때 만들어둔 clsx 유틸 사용하시면 됩니다!
- clxs 사용 - borderPlacement 기본값 적용
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/Input/MultiInput.tsx (1)
47-50: Consider using clsx more effectively.The current implementation could be simplified using clsx's object syntax.
-<div className={clsx( - inputFieldsContainerStyle, - interactableBorderStyle({ isValid }))} -> +<div className={clsx(inputFieldsContainerStyle, { + [interactableBorderStyle({ isValid })]: borderPlacement === 'container' +})} +>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Input/Core/InputField.tsx(3 hunks)frontend/src/components/Input/MultiInput.tsx(3 hunks)
🧰 Additional context used
📓 Learnings (3)
📓 Common learnings
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#100
File: frontend/src/components/Input/MultiInput.tsx:19-22
Timestamp: 2025-02-06T02:51:01.268Z
Learning: In the Input component, the `borderPlacement` prop should be optional with a default value of 'inputField' to maintain backward compatibility and improve developer experience.
frontend/src/components/Input/Core/InputField.tsx (1)
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#100
File: frontend/src/components/Input/MultiInput.tsx:19-22
Timestamp: 2025-02-06T02:51:01.268Z
Learning: In the Input component, the `borderPlacement` prop should be optional with a default value of 'inputField' to maintain backward compatibility and improve developer experience.
frontend/src/components/Input/MultiInput.tsx (1)
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#100
File: frontend/src/components/Input/MultiInput.tsx:19-22
Timestamp: 2025-02-06T02:51:01.268Z
Learning: In the Input component, the `borderPlacement` prop should be optional with a default value of 'inputField' to maintain backward compatibility and improve developer experience.
🔇 Additional comments (6)
frontend/src/components/Input/Core/InputField.tsx (3)
4-10: LGTM! Import paths have been updated to use absolute paths.The changes improve maintainability by using absolute imports and add necessary styling utilities.
21-21: LGTM! Context usage is aligned with the new feature.The
borderPlacementis correctly destructured from the context alongside existing properties.
30-35: LGTM! Conditional styling is implemented correctly.The use of
clsxfor conditional class names and the border style application based onborderPlacementis well implemented.frontend/src/components/Input/MultiInput.tsx (3)
4-17: LGTM! Import organization and utility usage are well structured.The changes improve code organization by:
- Using the
clsxutility for class name management- Importing core components from their new locations
- Adding necessary style imports
20-23: LGTM! Props interface correctly defines optional borderPlacement.The
borderPlacementprop is correctly defined as optional, aligning with the retrieved learning about maintaining backward compatibility.
25-35: LGTM! Default value implementation maintains backward compatibility.The default value of 'inputField' for
borderPlacementensures backward compatibility as suggested in the retrieved learning.
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
borderPlacement = 'container' | 'inputField'prop을 통해 border 설정 위치를 조정할 수 있습니다.🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Summary by CodeRabbit
intersperseElementfunction to manage child elements with separators in theMultiInputcomponent.interactableBorderStyleto enhance border rendering based on validity.