[FE-Feat] 일정 추가, 수정 팝오버 컴포넌트 구현 #101
Conversation
WalkthroughSeveral UI components have been updated and new ones introduced. The Button and Text components now accept an optional Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SP as SchedulePopover
participant T as Title
participant PF as PopoverForm
participant PB as PopoverButton
U->>SP: Open SchedulePopover (isOpen=true)
SP->>T: Render title based on type
SP->>PF: Render the scheduling form
SP->>PB: Render button(s) (conditional on popover type)
U->>PB: Click Save or Delete button
PB->>SP: Trigger onClickSave/onClickDelete handler
SP-->>U: Update schedule state / close popover
Assessment against linked issues
Possibly related PRs
Poem
🪧 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: 5
🧹 Nitpick comments (9)
frontend/src/components/Toggle/Toggle.stories.tsx (1)
45-51: Add return type annotations for story components.The story components are missing return type annotations. This helps with type safety and documentation.
Apply this diff to add return type annotations:
-export const ControlledCheck = () => { +export const ControlledCheck = (): JSX.Element => { -export const ControlledGroupCheck = () => { +export const ControlledGroupCheck = (): JSX.Element => {Also applies to: 53-77
frontend/src/components/Text/index.tsx (1)
3-4: LGTM! Well-implemented className support.The implementation follows React best practices and maintains backward compatibility. The use of clsx for class composition is appropriate.
Consider adding JSDoc comments to document the purpose of the className prop:
interface TextProps extends PropsWithChildren { typo?: Typo; color?: string; + /** Additional CSS classes to be applied to the text element */ className?: string; }Also applies to: 12-12, 15-16
frontend/src/components/Icon/component/Plus.tsx (1)
4-11: Consider enhancing accessibility attributes.The implementation looks good, but consider adding
role="img"to the SVG for better screen reader support.- <svg width={width} height={width} viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg" aria-label="plus icon" fill="none" className={className} style={{ cursor: clickable ? "pointer": "default", ...rest.style }} {...rest}> + <svg width={width} height={width} viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg" aria-label="plus icon" role="img" fill="none" className={className} style={{ cursor: clickable ? "pointer": "default", ...rest.style }} {...rest}>frontend/src/routes/my-schedule/@components/SchedulePopover/index.tsx (1)
21-32: Enhance dialog accessibility attributes.While the dialog implementation is good, consider adding ARIA attributes for better accessibility.
- <dialog className={containerStyle} open={isOpen}> + <dialog + className={containerStyle} + open={isOpen} + aria-modal="true" + aria-labelledby="schedule-popover-title" + > - <Title type={type} /> + <Title type={type} id="schedule-popover-title" />frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverButton.tsx (1)
13-31: Consider using i18n for button labels.The Korean text labels are hardcoded. Consider using an i18n solution for better internationalization support.
Example using a translation hook:
+ import { useTranslation } from 'react-i18next'; export const PopoverButton = ({ type, ...handlers }: PopoverButtonProps)=>{ + const { t } = useTranslation(); return ( - type === 'add' ? <Button className={buttonStyle} onClick={handlers.onClickSave}>저장</Button> : ( + type === 'add' ? <Button className={buttonStyle} onClick={handlers.onClickSave}>{t('common.save')}</Button> : ( <Flex gap={100} justify='flex-end' width='100%' > <Button className={buttonStyle} onClick={handlers.onClickDelete} style='weak' type='destructive' > - 삭제 + {t('common.delete')} </Button> - <Button className={buttonStyle} onClick={handlers.onClickSave}>저장</Button> + <Button className={buttonStyle} onClick={handlers.onClickSave}>{t('common.save')}</Button> </Flex> ) ); }frontend/src/routes/my-schedule/@components/SchedulePopover/Title.tsx (1)
10-24: Move map outside component and use i18n.Consider these improvements:
- Move the map outside the component to prevent recreation on each render
- Use i18n for title text
+ import { useTranslation } from 'react-i18next'; + const POPOVER_TITLE_TYPE_MAP: Record<PopoverType, { + titleKey: string; + icon: (color: string) => ReactNode; + }> = { + add: { + titleKey: 'schedule.add', + icon: (color) => <Plus stroke={color} width={20} />, + }, + edit: { + titleKey: 'schedule.edit', + icon: (color) => <Pencil fill={color} width={16} />, + }, + }; export const Title = ({ type }: { type: PopoverType })=>{ + const { t } = useTranslation(); + const color = vars.color.Ref.Netural[600]; - const popoverTitleTypeMap: Record<PopoverType, { - title: string; - icon: ReactNode; - }> = { - add: { - title: '일정 추가', - icon: <Plus stroke={vars.color.Ref.Netural[600]} width={20} />, - }, - edit: { - title: '일정 수정', - icon: <Pencil fill={vars.color.Ref.Netural[600]} width={16} />, - }, - };frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverForm.tsx (1)
18-18: Replace raw input with Input componentFor consistency, use the
Inputcomponent instead of raw HTML input to maintain UI consistency and inherit built-in features.- <input className={inputStyle} placeholder='새 일정' /> + <Input placeholder='새 일정' />frontend/src/components/Button/index.tsx (1)
18-18: Add JSDoc documentation for className propAdd documentation to explain the purpose and usage of the className prop.
+ /** Additional CSS class names to be applied to the button */ className?: string;frontend/src/routes/my-schedule/@components/SchedulePopover/index.css.ts (1)
6-22: Consider accessibility implications of blur effectThe blur effect might cause readability issues for some users. Consider adding a media query to respect user preferences.
export const containerStyle = style({ width: 262, padding: vars.spacing[300], display: 'flex', flexDirection: 'column', gap: vars.spacing[100], backgroundColor: '#E5E8EB80', - backdropFilter: 'blur(20px)', + '@media (prefers-reduced-transparency: reduce)': { + backgroundColor: vars.color.Ref.Netural[100], + backdropFilter: 'none', + }, + '@media (prefers-reduced-transparency: no-preference)': { + backdropFilter: 'blur(20px)', + }, boxShadow: '0px 0px 15px rgba(0, 0, 0, 0.15)', border: `1px solid ${vars.color.Ref.Netural[200]}`, borderRadius: vars.radius[500], });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/src/components/Icon/svg/plus.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
frontend/src/components/Button/index.tsx(3 hunks)frontend/src/components/Icon/component/Plus.tsx(1 hunks)frontend/src/components/Icon/index.ts(1 hunks)frontend/src/components/Input/Core/HelperText.tsx(1 hunks)frontend/src/components/Input/MultiInput.tsx(1 hunks)frontend/src/components/Input/SingleInput.tsx(1 hunks)frontend/src/components/Text/index.tsx(1 hunks)frontend/src/components/Toggle/Toggle.stories.tsx(1 hunks)frontend/src/components/Toggle/index.css.ts(1 hunks)frontend/src/components/Toggle/index.tsx(1 hunks)frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverButton.tsx(1 hunks)frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverForm.tsx(1 hunks)frontend/src/routes/my-schedule/@components/SchedulePopover/Title.tsx(1 hunks)frontend/src/routes/my-schedule/@components/SchedulePopover/index.css.ts(1 hunks)frontend/src/routes/my-schedule/@components/SchedulePopover/index.tsx(1 hunks)frontend/src/routes/my-schedule/@components/index.tsx(1 hunks)frontend/src/routes/my-schedule/@types/index.ts(1 hunks)frontend/src/theme/reset.css.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Send PR Notification to Reviewers
🔇 Additional comments (15)
frontend/src/components/Input/Core/HelperText.tsx (2)
10-14: LGTM! Good separation of concerns.The renaming of the original
HelperTexttoHelperTextItemimproves clarity by better describing its role as a presentational component.
16-22: LGTM! Clean validation logic.The new
HelperTextcomponent effectively handles the conditional rendering of helper text based on validation state. The order of conditions is correct, prioritizing error messages over hints.frontend/src/components/Input/SingleInput.tsx (1)
35-39: LGTM! Simplified helper text rendering.The direct usage of
HelperTextcomponent with props makes the code more maintainable and easier to understand compared to the previous separate rendering function approach.frontend/src/components/Input/MultiInput.tsx (1)
54-58: LGTM! Consistent helper text implementation.The implementation matches the SingleInput component, maintaining consistency across the codebase. This unified approach to helper text rendering improves maintainability.
frontend/src/components/Toggle/index.css.ts (1)
6-57: LGTM! Well-structured styles using vanilla-extract.The styles are well-organized using vanilla-extract's recipe pattern for variant-based styling. The toggle's visual states and transitions are properly handled.
frontend/src/components/Toggle/Toggle.stories.tsx (1)
1-79: LGTM! Comprehensive stories covering various use cases.The stories effectively demonstrate different usage patterns of the Toggle component:
- Single toggle with label
- Controlled toggle with state management
- Group of toggles with complex state management
frontend/src/components/Toggle/index.tsx (1)
1-55: LGTM! Well-implemented toggle component.The component is well-structured with:
- Clear prop interface with good documentation
- Proper state management using useCheckbox hook
- Flexible implementation supporting both controlled and uncontrolled modes
frontend/src/routes/my-schedule/@types/index.ts (1)
1-1: LGTM! Clear and well-defined type.The
PopoverTypetype alias is well-defined and follows TypeScript best practices.frontend/src/routes/my-schedule/@components/index.tsx (1)
4-6: Remove hardcoded values before production.The
isOpenprop is hardcoded totrue, which means the popover will always be visible. This appears to be for development/testing purposes.Consider implementing proper state management for the
isOpenprop based on user interactions. For example:- <SchedulePopover isOpen={true} type='edit' /> + <SchedulePopover isOpen={isPopoverOpen} type={popoverType} />frontend/src/components/Icon/index.ts (1)
9-9: LGTM! Well-organized icon export.The new Plus icon export follows the established pattern and maintains alphabetical ordering.
frontend/src/components/Icon/component/Plus.tsx (1)
13-13: LGTM!Setting displayName is good for debugging purposes.
frontend/src/routes/my-schedule/@components/SchedulePopover/index.tsx (2)
8-11: LGTM!Props interface is well-defined with clear types.
14-19: Implement the placeholder handlers.The handlers are currently empty. Please implement the save and delete functionality.
Would you like me to help implement these handlers based on your requirements?
frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverButton.tsx (1)
7-11: LGTM!Props interface is well-defined with clear handler types.
frontend/src/routes/my-schedule/@components/SchedulePopover/Title.tsx (1)
25-35: LGTM!The Text component implementation with icon and title rendering looks good.
frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverForm.tsx
Show resolved
Hide resolved
frontend/src/routes/my-schedule/@components/SchedulePopover/index.css.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/Toggle/index.tsx (1)
8-15: Consider making thevalueprop more flexible.The
valueprop is currently typed asnumber?, but toggles often need to work with various value types.Consider updating the interface to use a generic type parameter:
-interface ToggleProps { +interface ToggleProps<T = number> { - value?: number; + value?: T; isChecked?: boolean; onToggleCheck?: (prev: SetStateAction<boolean>) => void; type?: 'all' | 'single'; defaultChecked?: boolean; inputProps?: InputHTMLAttributes<HTMLInputElement>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/Toggle/index.tsx(1 hunks)
🔇 Additional comments (3)
frontend/src/components/Toggle/index.tsx (3)
17-26: Well-documented component!The JSDoc documentation clearly describes the component and all its props.
27-61: Clean implementation with good practices!The component is well-implemented with:
- Unique ID generation using
useId- Custom hook for state management
- Proper props spreading order
43-59: Accessibility attributes have been properly implemented.The component now includes all necessary accessibility attributes as previously suggested:
role="switch"aria-checkedaria-hidden="true"on the input element
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
Input컴포넌트의 헬퍼텍스트 로직을 수정합니다.🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Input이 멀티일 때 헬퍼텍스트가 없음에도 여전히 돔이 들어있는 문제가 있어서,HelperText컴포넌트 내부에서 분기처리 하는 식으로 수정하여Single과Multi모두 적용했습니다. 문제 있는지 확인 부탁드려요!Summary by CodeRabbit
New Features
Refactor