[FE-Feat] 일정 조율 생성 완료 페이지 구현 #116
Conversation
WalkthroughThis PR introduces multiple enhancements and new features. The Button and Flex components have been updated to support polymorphism via the AsProp type. A new Divider component is added alongside its Storybook configuration and styling. A useClipboard hook is implemented for link copy functionality. A set of discussion creation components and routes is introduced, including a dedicated finish page and a new discussion creation route. Dropdown components now include enhanced state management. The root route has been updated to wrap components with a NotificationProvider. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as Router
participant DCFP as DiscussionCreateFinishPage
participant DCC as DiscussionCreateCard
participant UC as useClipboard
participant N as NotificationContext
U->>R: Navigate to /discussion/create/123
R->>DCFP: Render DiscussionCreateFinishPage
DCFP->>DCC: Render DiscussionCreateCard
U->>DCC: Click Share Button
DCC->>UC: Invoke handleCopyToClipboard("논의 주소")
UC->>navigator.clipboard: writeText()
navigator.clipboard-->>UC: Return copy status
UC->>N: Trigger notification
sequenceDiagram
participant U as User
participant DD as Dropdown Component
participant DC as Dropdown Context
U->>DD: Click trigger to open dropdown
DD->>DC: Toggle isOpen (true)
U->>DD: Select dropdown item
DD->>DC: Call setIsOpen(false) to close dropdown
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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/routes/discussion/create/$id.tsx (1)
3-3: Consider using absolute imports for better maintainability.Replace the relative import with an absolute import using the
@alias:-import DiscussionCreateFinishPage from '../@components'; +import DiscussionCreateFinishPage from '@/routes/discussion/@components';frontend/src/components/Divider/index.css.ts (1)
3-9: Add type definitions for theme variables.Consider adding type definitions to improve type safety and developer experience:
+type DividerVars = { + divider: { + width: string; + height: string; + color: string; + } +}; -export const dividerVars = createThemeContract({ +export const dividerVars = createThemeContract<DividerVars>({ divider: { width: null, height: null, color: null, }, });frontend/src/routes/discussion/@components/index.tsx (1)
6-13: Enhance accessibility with semantic HTML.Consider using semantic HTML elements to improve accessibility:
const DiscussionCreateFinishPage = () => ( - <Flex + <Flex + as="main" align='center' className={pageContainerStyle} > <DiscussionCreateCard /> </Flex> );This change:
- Makes the page structure more semantic
- Improves screen reader navigation
- Better conveys the content's purpose
frontend/src/routes/discussion/@components/DiscussionCreateCard/index.css.ts (1)
5-11: Consider making the width responsive.The hardcoded width of '31.125rem' might not provide the best experience across different screen sizes.
Consider using relative units or media queries:
export const containerStyle = style({ - width: '31.125rem', + width: '100%', + maxWidth: '31.125rem', padding: vars.spacing[800], borderRadius: vars.spacing[800], background: 'linear-gradient(180deg, #FFFFFF66 0%, #FFFFFF99 46.5%, #FFFFFFCC 100%)', });frontend/src/components/Divider/Divider.stories.tsx (1)
7-11: Enhance Storybook configuration with prop controls.The current configuration could be improved by:
- Adding a more specific meta type
- Defining prop controls in argTypes
-const meta: Meta = { +const meta = { title: 'Components/Divider', component: Divider, - argTypes: {}, + argTypes: { + width: { + control: { type: 'text' }, + description: 'Width of the divider', + }, + height: { + control: { type: 'number' }, + description: 'Height of the divider in pixels', + }, + color: { + control: { type: 'color' }, + description: 'Color of the divider', + }, + }, } satisfies Meta<typeof Divider>;frontend/src/components/Flex/Flex.stories.tsx (1)
32-41: Enhance HeaderFlex story with realistic header content.While the story effectively demonstrates the polymorphic behavior, consider making it more representative of a real header layout.
export const HeaderFlex: Story = { args: { width: 'full', direction: 'row', justify: 'space-between', as: 'header', align: 'center', - children: <div>hello world</div>, + children: ( + <> + <div>Logo</div> + <nav> + <div>Menu Item 1</div> + <div>Menu Item 2</div> + </nav> + </> + ), }, };frontend/src/components/Divider/index.tsx (1)
8-15: Add validation for numeric width/height props.Consider adding validation to ensure width and height are non-negative when provided as numbers.
interface DividerProps { width?: Width; height?: number | string; color?: string; className?: string; + 'aria-orientation'?: 'horizontal' | 'vertical'; }frontend/src/routes/discussion/@components/DiscussionCreateCard/index.tsx (1)
16-18: Extract hardcoded string to constant.Move the hardcoded string '논의 주소' to a constant for better maintainability.
+const DISCUSSION_ADDRESS = '논의 주소'; + const handleClickShareButton = () => { - handleCopyToClipboard('논의 주소'); + handleCopyToClipboard(DISCUSSION_ADDRESS); };frontend/src/components/Flex/index.tsx (1)
42-55: Consider memoizing style variables for performance.The style object is recreated on every render. Consider memoizing it if the component is used in performance-critical scenarios.
+ import { useMemo } from 'react'; // ...inside component - return ( - <Component - className={clsx(className, flexStyle)} - style={assignInlineVars(flexVars, { - flex: { - width: formattedWidth, - height: typeof height === 'number' ? `${height}px` : height, - direction, - justify, - align, - gap: gap ? vars.spacing[gap] : '0', - }, - })} - {...props} - > + const styles = useMemo(() => + assignInlineVars(flexVars, { + flex: { + width: formattedWidth, + height: typeof height === 'number' ? `${height}px` : height, + direction, + justify, + align, + gap: gap ? vars.spacing[gap] : '0', + }, + }), + [formattedWidth, height, direction, justify, align, gap] + ); + + return ( + <Component + className={clsx(className, flexStyle)} + style={styles} + {...props} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 086f511 and f2797158d6229fe17eec0eed1b605ce7af904dc1.
📒 Files selected for processing (17)
frontend/src/components/Button/index.tsx(2 hunks)frontend/src/components/Divider/Divider.stories.tsx(1 hunks)frontend/src/components/Divider/index.css.ts(1 hunks)frontend/src/components/Divider/index.tsx(1 hunks)frontend/src/components/Flex/Flex.stories.tsx(1 hunks)frontend/src/components/Flex/index.tsx(4 hunks)frontend/src/hooks/useClipboard.ts(1 hunks)frontend/src/routeTree.gen.ts(6 hunks)frontend/src/routes/__root.tsx(2 hunks)frontend/src/routes/discussion/@components/DiscussionCreateCard/DiscussionInfo.tsx(1 hunks)frontend/src/routes/discussion/@components/DiscussionCreateCard/index.css.ts(1 hunks)frontend/src/routes/discussion/@components/DiscussionCreateCard/index.tsx(1 hunks)frontend/src/routes/discussion/@components/index.css.ts(1 hunks)frontend/src/routes/discussion/@components/index.tsx(1 hunks)frontend/src/routes/discussion/create/$id.tsx(1 hunks)frontend/src/routes/landing/@components/index.tsx(2 hunks)frontend/src/types/polymorphism.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/src/routes/discussion/@components/index.css.ts
- frontend/src/routes/landing/@components/index.tsx
🔇 Additional comments (8)
frontend/src/types/polymorphism.ts (1)
1-5: Well-structured polymorphic type definition!The
AsProptype is well-designed:
- Properly extends React's
ElementType- Enables type-safe polymorphic components
- Follows TypeScript best practices
frontend/src/components/Divider/index.css.ts (1)
5-7: Avoid using null for theme variables.Using
nullas initial values could lead to runtime styling issues. Consider using default values or CSS variables:divider: { - width: null, - height: null, - color: null, + width: 'var(--divider-width, 100%)', + height: 'var(--divider-height, 1px)', + color: 'var(--divider-color, #e0e0e0)', },frontend/src/routes/__root.tsx (1)
20-23: LGTM! Well-placed NotificationProvider.The NotificationProvider is correctly positioned at the root level, ensuring notification functionality is available throughout the application.
frontend/src/routes/discussion/@components/DiscussionCreateCard/index.tsx (1)
41-41: Add error handling for clipboard operation.Consider handling potential clipboard operation failures.
frontend/src/components/Button/index.tsx (1)
8-8: LGTM! Well-implemented polymorphic component pattern.The changes effectively implement the polymorphic component pattern by:
- Importing the shared
AsProptype- Adding props spreading support
- Properly applying spread props to the rendered component
This enhances the component's reusability while maintaining type safety.
Also applies to: 38-38, 46-46
frontend/src/components/Flex/index.tsx (1)
20-31: LGTM! Well-structured polymorphic component with proper defaults.The component signature is well-organized with:
- Clear type constraints
- Sensible default values
- Proper type composition using
AsProp,ComponentPropsWithoutRef, andFlexPropsfrontend/src/routeTree.gen.ts (2)
46-50: LGTM! Route structure follows best practices.The discussion creation route is well-structured with:
- Dynamic parameter
$idin the path- Proper parent route configuration
- Consistent route definition pattern
77-83: Verify the route parameter type definition.The route uses a dynamic
$idparameter. Ensure that the parameter type is properly defined in the route component file.Also applies to: 93-93, 100-100, 108-108, 113-121
frontend/src/routes/discussion/@components/DiscussionCreateCard/index.css.ts
Outdated
Show resolved
Hide resolved
frontend/src/routes/discussion/@components/DiscussionCreateCard/DiscussionInfo.tsx
Outdated
Show resolved
Hide resolved
frontend/src/routes/discussion/@components/DiscussionCreateCard/index.tsx
Outdated
Show resolved
Hide resolved
hamo-o
force-pushed
the
feature/fe/discussion-finish-markup
branch
from
82707de to
1180836
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
frontend/src/components/Button/index.tsx (1)
38-38: Ensure props spreading doesn't override important attributes.Props spreading could potentially override className or onClick. Consider moving the spread before the explicit props:
<Component + {...props} className={clsx(containerStyle({ variant, style, radius, size }), className)} onClick={onClick} - {...props} >Also applies to: 46-46
frontend/src/components/Flex/index.tsx (2)
20-31: Consider moving default values to interface level.The default values could be defined at the interface level using TypeScript's built-in utility types for better type inference and documentation.
interface FlexProps extends PropsWithChildren { width?: number | string | 'full' | 'auto'; // Include 'auto' in type height?: number | string | 'auto'; // Include 'auto' in type direction?: 'row' | 'column' /* = 'row' */; justify?: 'center' | 'flex-start' | 'flex-end' | 'space-between' | 'space-around' /* = 'center' */; align?: 'center' | 'flex-start' | 'flex-end' /* = 'flex-start' */; gap?: keyof typeof vars.spacing; className?: string; }
42-55: Consider props spreading position for style precedence.Props spreading could potentially override the style prop. Consider moving the spread before the explicit props:
<Component + {...props} className={clsx(className, flexStyle)} style={assignInlineVars(flexVars, { flex: { width: formattedWidth, height: typeof height === 'number' ? `${height}px` : height, direction, justify, align, gap: gap ? vars.spacing[gap] : '0', }, })} - {...props} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 82707def43a7ee8ab5f4e8ac9e5e120173fda7f0 and 1180836.
📒 Files selected for processing (20)
frontend/src/components/Button/index.tsx(2 hunks)frontend/src/components/Divider/Divider.stories.tsx(1 hunks)frontend/src/components/Divider/index.css.ts(1 hunks)frontend/src/components/Divider/index.tsx(1 hunks)frontend/src/components/Dropdown/DropdownContext.ts(2 hunks)frontend/src/components/Dropdown/DropdownItem.tsx(1 hunks)frontend/src/components/Dropdown/index.css.ts(0 hunks)frontend/src/components/Dropdown/index.tsx(1 hunks)frontend/src/components/Flex/Flex.stories.tsx(1 hunks)frontend/src/components/Flex/index.tsx(4 hunks)frontend/src/hooks/useClipboard.ts(1 hunks)frontend/src/routeTree.gen.ts(7 hunks)frontend/src/routes/__root.tsx(2 hunks)frontend/src/routes/discussion/@components/DiscussionCreateCard/DiscussionInfo.tsx(1 hunks)frontend/src/routes/discussion/@components/DiscussionCreateCard/index.css.ts(1 hunks)frontend/src/routes/discussion/@components/DiscussionCreateCard/index.tsx(1 hunks)frontend/src/routes/discussion/@components/index.css.ts(1 hunks)frontend/src/routes/discussion/@components/index.tsx(1 hunks)frontend/src/routes/discussion/create/$id.tsx(1 hunks)frontend/src/types/polymorphism.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/Dropdown/index.css.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- frontend/src/routes/discussion/create/$id.tsx
- frontend/src/components/Flex/Flex.stories.tsx
- frontend/src/components/Divider/index.css.ts
- frontend/src/routes/discussion/@components/DiscussionCreateCard/index.css.ts
- frontend/src/types/polymorphism.ts
- frontend/src/routes/discussion/@components/index.css.ts
- frontend/src/components/Divider/Divider.stories.tsx
- frontend/src/routes/discussion/@components/DiscussionCreateCard/index.tsx
- frontend/src/routes/discussion/@components/index.tsx
- frontend/src/routes/__root.tsx
- frontend/src/components/Divider/index.tsx
🧰 Additional context used
📓 Learnings (1)
frontend/src/routes/discussion/@components/DiscussionCreateCard/DiscussionInfo.tsx (1)
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#116
File: frontend/src/routes/discussion/@components/DiscussionCreateCard/DiscussionInfo.tsx:7-21
Timestamp: 2025-02-07T11:41:18.060Z
Learning: For components that consume backend data, interface definitions should be derived from the backend DTOs to maintain consistency between frontend and backend types.
🔇 Additional comments (10)
frontend/src/components/Dropdown/DropdownContext.ts (1)
3-8: LGTM! Well-documented context interface.The addition of
setIsOpento theDropdownContextPropsinterface is well-typed and properly integrated.frontend/src/components/Dropdown/DropdownItem.tsx (1)
14-21: LGTM! Clean implementation of dropdown item selection.The implementation correctly handles item selection and dropdown closure.
frontend/src/components/Button/index.tsx (2)
8-8: LGTM! Good practice importing the shared AsProp type.Moving the AsProp type to a shared location promotes reusability and consistency across components.
27-39: Consider type safety for onClick handler with polymorphic components.The onClick handler is typed specifically for HTMLButtonElement, but the component is now polymorphic. This could cause type errors when using non-button elements.
Consider updating the onClick type to be more flexible:
- onClick?: MouseEventHandler<HTMLButtonElement>; + onClick?: MouseEventHandler<Element>;✅ Verification successful
Action Required: Update onClick Handler Type for Polymorphic Button Component
The current onClick type (MouseEventHandler) is too restrictive given that the component is polymorphic. Changing it to MouseEventHandler will ensure type safety when users render non‑button elements via the “as” prop.
• File: frontend/src/components/Button/index.tsx (around lines 27–39)
• Update the onClick type from MouseEventHandler to MouseEventHandlerfrontend/src/components/Flex/index.tsx (1)
35-39: LGTM! Clean implementation of width formatting.The IIFE for width formatting is a clean way to handle the different width types.
frontend/src/hooks/useClipboard.ts (2)
7-21:⚠️ Potential issueImprove error handling and clipboard API support.
The current implementation needs improvements in error handling and browser support:
const handleCopyToClipboard = async (text: string) => { + if (!navigator?.clipboard) { + addNoti({ + title: '클립보드가 지원되지 않는 브라우저입니다', + type: 'error', + }); + return; + } + // TODO: 노티피케이션 다루는 공통 로직 try { await navigator.clipboard.writeText(text); addNoti({ title: '링크 복사가 완료됐어요', type: 'success', }); - } catch (error) { + } catch (error: unknown) { + console.error('Clipboard error:', error); addNoti({ title: '링크 복사에 실패했어요', type: 'error', }); } };Likely invalid or redundant comment.
8-8: Address TODO comment about notification handling.Let's verify if there's a common pattern for notification handling in the codebase.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for notification handling patterns rg "addNoti" -A 3 -B 3Length of output: 3776
Notification Handling Pattern Verified
- The useClipboard hook already leverages the common notification handling pattern by retrieving addNoti from the shared NotificationContext via useSafeContext.
- This pattern is consistently implemented in NotificationContext.ts, NotificationProvider.tsx, and useNotification.ts.
- The TODO comment ("// TODO: 노티피케이션 다루는 공통 로직") appears to be a reminder for potential future refactoring. If the current implementation meets the requirements, consider removing or updating the comment to reflect the existing common handling logic.
frontend/src/routes/discussion/@components/DiscussionCreateCard/DiscussionInfo.tsx (2)
14-14:⚠️ Potential issueFix typo in color variable name.
There's a typo in the color variable name: "Netural" should be "Neutral".
- <Text color={vars.color.Ref.Netural[800]} typo='h3'>기업디(3) 첫 팀플</Text> + <Text color={vars.color.Ref.Neutral[800]} typo='h3'>기업디(3) 첫 팀플</Text>Likely invalid or redundant comment.
7-21: Verify if text content should be externalized.The component contains hardcoded text content. While we'll wait for the backend DTO to define the interface, consider if the text content should be externalized for better maintainability and potential internationalization.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if there's a pattern of using i18n or text constants in the codebase rg -g "*.{ts,tsx}" "import.*['\"]\.\/.*\.strings['\"]" || rg -g "*.{ts,tsx}" "import.*['\"]\.\/.*\.i18n['\"]"Length of output: 339
I'll re-run a corrected shell script to verify if the codebase uses externalized strings (e.g. via *.strings or *.i18n imports) in TypeScript/TSX files:
This should help us determine if text content has been externalized elsewhere.
🏁 Script executed:
#!/bin/bash echo "Searching for '.strings' in TS and TSX files" rg -g "*.{ts,tsx}" '\.strings' echo "Searching for '.i18n' in TS and TSX files" rg -g "*.{ts,tsx}" '\.i18n'Length of output: 266
Externalized text verification
Based on the searches performed for externalized strings (using patterns for both “.strings” and “.i18n”), there is no evidence that text content is externalized anywhere in the codebase. Thus, the hardcoded text in DiscussionInfo.tsx remains intentional for now (pending the backend DTO).
• No import patterns for externalized locales or string constants were found in TS/TSX files.
• The hardcoded text appears consistent with the current approach while waiting for backend specifications.frontend/src/routeTree.gen.ts (1)
1-204: Skip review of auto-generated file.This file is automatically generated by TanStack Router and should not be modified manually as indicated by the file header comments.
| <div | ||
| className={dropdownContainerStyle} | ||
| id={defaultId} | ||
| style={{ width }} | ||
| > | ||
| <div onClick={() => setIsOpen((prev) => !prev)}> | ||
| {trigger} | ||
| </div> | ||
| { | ||
| isOpen && | ||
| <ul className={dropdownContentStyle} style={{ height }}> | ||
| {children} | ||
| </ul> | ||
| } | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance keyboard accessibility.
The dropdown should be navigable via keyboard for better accessibility.
Add keyboard event handlers:
<div
className={dropdownContainerStyle}
id={defaultId}
style={{ width }}
+ role="combobox"
+ aria-expanded={isOpen}
+ aria-haspopup="listbox"
+ tabIndex={0}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ setIsOpen((prev) => !prev);
+ } else if (e.key === 'Escape' && isOpen) {
+ setIsOpen(false);
+ }
+ }}
>
<div onClick={() => setIsOpen((prev) => !prev)}>
{trigger}
</div>
{
isOpen &&
- <ul className={dropdownContentStyle} style={{ height }}>
+ <ul
+ className={dropdownContentStyle}
+ style={{ height }}
+ role="listbox"
+ aria-labelledby={defaultId}
+ >
{children}
</ul>
}
</div>📝 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={dropdownContainerStyle} | |
| id={defaultId} | |
| style={{ width }} | |
| > | |
| <div onClick={() => setIsOpen((prev) => !prev)}> | |
| {trigger} | |
| </div> | |
| { | |
| isOpen && | |
| <ul className={dropdownContentStyle} style={{ height }}> | |
| {children} | |
| </ul> | |
| } | |
| </div> | |
| <div | |
| className={dropdownContainerStyle} | |
| id={defaultId} | |
| style={{ width }} | |
| role="combobox" | |
| aria-expanded={isOpen} | |
| aria-haspopup="listbox" | |
| tabIndex={0} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| setIsOpen((prev) => !prev); | |
| } else if (e.key === 'Escape' && isOpen) { | |
| setIsOpen(false); | |
| } | |
| }} | |
| > | |
| <div onClick={() => setIsOpen((prev) => !prev)}> | |
| {trigger} | |
| </div> | |
| { | |
| isOpen && | |
| <ul | |
| className={dropdownContentStyle} | |
| style={{ height }} | |
| role="listbox" | |
| aria-labelledby={defaultId} | |
| > | |
| {children} | |
| </ul> | |
| } | |
| </div> |
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
Flex다형성 고려하기2025-02-07.3.10.26.mov
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
논의 URL 생성 로직 구현은 생성 폼에서 진행하겠습니다. (생성 버튼 클릭 시 URL 생성, 성공 시 이 페이지로 라우팅 되는 게 맞는 것 같습니다.)
Summary by CodeRabbit