Skip to content

Conversation

@dioo1461
Copy link
Contributor

@dioo1461 dioo1461 commented Feb 10, 2025

#️⃣ 연관된 이슈>

📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)

일정 조율 초대 화면 구현

  • canJoin: true

image

  • canJoin: false

image

Button 컴포넌트에 disabled 속성 주입 기능 구현

  • Button을 제네릭으로 다른 타입으로 변환해 쓸 경우에도 disable 속성이 제대로 작동할 수 있도록 구현했습니다.
  • 쓸 때는 그냥 Button에 disabled 속성 주면 됩니다.
<Button 
  as='div'
  ...
  disabled={true}
>
  {buttonText}
</Button>

🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

Summary by CodeRabbit

  • New Features

    • Enhanced button behavior with a clear disabled state for improved accessibility.
    • Introduced a discussion invite experience featuring a new invite card with badges displaying date ranges, times, durations, and optional location.
    • Launched a dedicated discussion invite page and dynamic routing for streamlined access to invitations.
  • Style

    • Refined visual presentation for buttons and discussion invite modals to deliver a more polished user interface.

@coderabbitai
Copy link

coderabbitai bot commented Feb 10, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

This pull request introduces several updates across UI components, routing, and utility functions. The Button component now supports a disabled state with updated styles and accessibility handling. New components and styles for discussion invitations are added, along with a dedicated invite page and a dynamic route. Utility functions for date and time are restructured, with some functions relocated and new formatting functions introduced. Additionally, unused imports have been removed to clean up the code.

Changes

File(s) Change Summary
frontend/src/components/Button/index.css.ts
frontend/src/components/Button/index.tsx
Added a disabled state variant in Button styles and updated ButtonProps with a new disabled property and accessibility handling.
frontend/src/components/DatePicker/Header/index.tsx Updated the import path for getDateParts from a specific calendar module to a more general date utilities module.
frontend/src/features/discussion/ui/DiscussionInviteCard/* Introduced new components (Badges, DiscussionInviteCard), new style recipes (e.g., modalFooterStyle with a disabled variant) and corresponding interfaces for discussion invite cards.
frontend/src/pages/DiscussionInvitePage/* Added a new DiscussionInvitePage component and an associated empty CSS file as a placeholder.
frontend/src/routeTree.gen.ts
frontend/src/routes/discussion/invite/$id.tsx
Added a new dynamic route for discussion invites, updating the route tree and exporting the corresponding component that utilizes a GlobalNavBar with discussion invite layouts.
frontend/src/utils/date/* Removed getDateParts from calendar module; added getDateParts, getDateRangeString in date.ts, and introduced time functions getTimeParts, getTimeRangeString in time.ts.
frontend/src/components/DatePicker/DatePicker.stories.tsx Removed unused imports related to hooks and context for a cleaner story configuration.
frontend/src/hooks/useDatePicker/useDatePickerSelect.ts Consolidated import statements without affecting functionality.

Sequence Diagram(s)

sequenceDiagram
  participant U as User
  participant B as Button Component
  participant A as accessibilityProps
  participant H as onClick Handler

  U->>B: Click event
  B->>A: Evaluate disabled state
  alt Button is disabled
    A-->>B: Return disabled props (aria-disabled, no onClick)
  else Button is enabled
    A-->>B: Return normal props
    B->>H: Process onClick event
  end
Loading
sequenceDiagram
  participant U as User
  participant DIP as DiscussionInvitePage
  participant DIC as DiscussionInviteCard
  participant GB as GlobalNavBar

  U->>DIP: Navigate to Invite Page
  DIP->>DIC: Pass dummyData props
  DIC->>GB: Render Global Navigation (MySchedule, NewDiscussion)
  DIC->>U: Display invite modal with badges & avatars
  U->>DIC: Click Join button (if enabled)
  DIC->>U: Trigger invitation acceptance alert
Loading

Possibly related PRs

Suggested reviewers

  • hamo-o

Poem

I’m a rabbit hopping in the code,
Bringing changes lightening your load.
Button disabled or invite so grand,
New routes and utils at our command.
A cheerful hop, a joyful beat,
Changes made so fresh and neat.
🐰🌟 Happy coding on this fun retreat!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9ae3cd83e74431efa5de5d79db469619e93dd34 and aa20d3b.

📒 Files selected for processing (15)
  • frontend/src/components/Button/index.css.ts (1 hunks)
  • frontend/src/components/Button/index.tsx (4 hunks)
  • frontend/src/components/DatePicker/DatePicker.stories.tsx (1 hunks)
  • frontend/src/components/DatePicker/Header/index.tsx (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/Badges.tsx (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/index.css.ts (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/index.tsx (1 hunks)
  • frontend/src/hooks/useDatePicker/useDatePickerSelect.ts (1 hunks)
  • frontend/src/pages/DiscussionInvitePage/index.css.ts (1 hunks)
  • frontend/src/pages/DiscussionInvitePage/index.tsx (1 hunks)
  • frontend/src/routeTree.gen.ts (9 hunks)
  • frontend/src/routes/discussion/invite/$id.tsx (1 hunks)
  • frontend/src/utils/date/calendar/index.ts (0 hunks)
  • frontend/src/utils/date/date.ts (1 hunks)
  • frontend/src/utils/date/time.ts (1 hunks)
✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@dioo1461 dioo1461 marked this pull request as ready for review February 10, 2025 06:56
@dioo1461 dioo1461 requested a review from hamo-o as a code owner February 10, 2025 06:56
@dioo1461 dioo1461 added the 🖥️ FE Frontend label Feb 10, 2025
@dioo1461 dioo1461 added this to the 2️⃣ 2차 스프린트 milestone Feb 10, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🧹 Nitpick comments (10)
frontend/src/components/Button/index.tsx (1)

44-54: Consider adding hover styles for non-disabled state.

The disabled state is correctly passed to both the className and accessibility props. However, you might want to ensure hover styles are only applied when the button is not disabled.

frontend/src/components/Button/index.css.ts (1)

61-69: Reconsider use of !important in styles.

While the disabled styles work, using !important is generally discouraged as it makes styles harder to maintain and override when needed. Consider:

  1. Restructuring the variant order
  2. Using compound variants for specific style combinations

Here's a suggested approach without !important:

    disabled: {
      true: {
        cursor: 'not-allowed',
-       backgroundColor: `${vars.color.Ref.Netural[300]} !important`,
-       color: `${vars.color.Ref.Netural['White']} !important`,
+       backgroundColor: vars.color.Ref.Netural[300],
+       color: vars.color.Ref.Netural['White'],
      },
      false: {},
    },

Then add compound variants for specific combinations:

// Add to compoundVariants array
{
  variants: {
    disabled: true,
    variant: 'primary',
  },
  style: {
    backgroundColor: vars.color.Ref.Netural[300],
    color: vars.color.Ref.Netural['White'],
  }
},
// Add similar compounds for other variants
frontend/src/components/Badge/Badge.stories.ts (1)

22-27: Add more stories to showcase different badge variants.

Consider adding stories for each icon type to better demonstrate the component's capabilities.

export const WithDateIcon: Story = {
  args: {
    children: '2024년 2월 15일',
    iconType: 'date',
  },
};

export const WithTimeIcon: Story = {
  args: {
    children: '오후 2시',
    iconType: 'time',
  },
};

export const WithLocationIcon: Story = {
  args: {
    children: '서울시 강남구',
    iconType: 'location',
  },
};

export const WithPersonIcon: Story = {
  args: {
    children: '4명',
    iconType: 'person',
  },
};
frontend/src/components/Icon/index.ts (1)

13-17: Maintain consistency in file extensions.

The TooltipArrow export is missing the .tsx extension while other exports include it.

Apply this diff to maintain consistency:

-export * from "./component/TooltipArrow";
+export * from "./component/TooltipArrow.tsx";
frontend/src/utils/date/time.ts (1)

15-23: Consider using Intl.DateTimeFormat for locale-aware time formatting.

The current implementation hardcodes Korean time format (, ). Consider using Intl.DateTimeFormat for better internationalization support.

Example implementation:

export const getTimeRangeString = (startTime: Date, endTime: Date): string => {
  if (!startTime || !endTime) {
    throw new Error('Both startTime and endTime are required');
  }
  
  const formatter = new Intl.DateTimeFormat('ko-KR', {
    hour: 'numeric',
    minute: 'numeric',
  });
  
  return `${formatter.format(startTime)} ~ ${formatter.format(endTime)}`;
};
frontend/src/components/Badge/index.tsx (2)

9-11: Consider adding props for icon customization.

The component could benefit from props to customize icon color and size.

Apply this diff to add customization props:

 interface BadgeProps extends PropsWithChildren {
   iconType?: 'date' | 'time' | 'location' | 'person' | 'none';
+  iconColor?: string;
+  iconSize?: number;
 }

37-42: Use custom props in getIcon function.

Update getIcon to use the custom icon props.

Apply this diff:

-const getIcon = (iconType: BadgeProps['iconType']) => {
+const getIcon = (
+  iconType: BadgeProps['iconType'],
+  iconColor?: string,
+  iconSize?: number,
+) => {
-  const WIDTH = 16;
-  const COLOR = vars.color.Ref.Netural[500];
   const IconComponent = ICON_MAPPING[iconType || 'none'];
-  return IconComponent ? <IconComponent fill={COLOR} width={WIDTH} /> : null;
+  return IconComponent ? (
+    <IconComponent
+      fill={iconColor ?? vars.color.Ref.Netural[500]}
+      width={iconSize ?? 16}
+    />
+  ) : null;
 };
frontend/src/features/discussion/ui/DiscussionInviteCard/Badges.tsx (2)

24-26: Consider using theme tokens for gap value

The gap value of 250 seems arbitrary. Consider using theme tokens or CSS variables for consistent spacing across the application.

-    gap={250}
+    gap={vars.space.lg}

29-30: Replace Korean comment with English and implement duration formatting

The comment about minute formatting should be in English, and the duration should be formatted to show hours and minutes when duration exceeds 60 minutes.

-    {/* 분 단위 포맷 구현 (~시간 ~분으로) */}
-    <Badge iconType='time'>{`${meetingDuration}분`}</Badge>
+    {/* Format duration in hours and minutes */}
+    <Badge iconType='time'>
+      {meetingDuration >= 60
+        ? `${Math.floor(meetingDuration / 60)}시간 ${meetingDuration % 60}분`
+        : `${meetingDuration}분`}
+    </Badge>
frontend/src/components/Icon/component/Clock.tsx (1)

6-6: Add preserveAspectRatio and role attributes to SVG

The SVG should preserve its aspect ratio and have an explicit role for accessibility.

-        <svg width={width} height={width} viewBox="0 0 24 24"  xmlns="http://www.w3.org/2000/svg" aria-label="clock icon" fill="none" className={className} style={{ cursor: clickable ? "pointer": "default", ...rest.style }} {...rest}>
+        <svg width={width} height={width} viewBox="0 0 24 24" preserveAspectRatio="xMidYMid meet" xmlns="http://www.w3.org/2000/svg" aria-label="clock icon" role="img" fill="none" className={className} style={{ cursor: clickable ? "pointer": "default", ...rest.style }} {...rest}>
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 95751d0 and a9ae3cd83e74431efa5de5d79db469619e93dd34.

⛔ Files ignored due to path filters (4)
  • frontend/src/components/Icon/svg/calendar-check.svg is excluded by !**/*.svg
  • frontend/src/components/Icon/svg/clock.svg is excluded by !**/*.svg
  • frontend/src/components/Icon/svg/pin-location.svg is excluded by !**/*.svg
  • frontend/src/components/Icon/svg/user-two.svg is excluded by !**/*.svg
📒 Files selected for processing (22)
  • frontend/src/components/Badge/Badge.stories.ts (1 hunks)
  • frontend/src/components/Badge/index.css.ts (1 hunks)
  • frontend/src/components/Badge/index.tsx (1 hunks)
  • frontend/src/components/Button/index.css.ts (1 hunks)
  • frontend/src/components/Button/index.tsx (4 hunks)
  • frontend/src/components/DatePicker/Header/index.tsx (1 hunks)
  • frontend/src/components/Icon/component/CalendarCheck.tsx (1 hunks)
  • frontend/src/components/Icon/component/Clock.tsx (1 hunks)
  • frontend/src/components/Icon/component/PinLocation.tsx (1 hunks)
  • frontend/src/components/Icon/component/UserTwo.tsx (1 hunks)
  • frontend/src/components/Icon/index.ts (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/Badges.tsx (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/index.css.ts (1 hunks)
  • frontend/src/features/discussion/ui/DiscussionInviteCard/index.tsx (1 hunks)
  • frontend/src/hooks/useDatePicker/index.ts (1 hunks)
  • frontend/src/pages/DiscussionInvitePage/index.css.ts (1 hunks)
  • frontend/src/pages/DiscussionInvitePage/index.tsx (1 hunks)
  • frontend/src/routeTree.gen.ts (9 hunks)
  • frontend/src/routes/discussion/invite/$id.tsx (1 hunks)
  • frontend/src/utils/date/calendar/index.ts (0 hunks)
  • frontend/src/utils/date/date.ts (1 hunks)
  • frontend/src/utils/date/time.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • frontend/src/utils/date/calendar/index.ts
✅ Files skipped from review due to trivial changes (3)
  • frontend/src/pages/DiscussionInvitePage/index.css.ts
  • frontend/src/components/DatePicker/Header/index.tsx
  • frontend/src/hooks/useDatePicker/index.ts
🔇 Additional comments (11)
frontend/src/components/Button/index.tsx (3)

15-26: LGTM! Good addition of the disabled prop.

The disabled prop is correctly added to the ButtonProps interface as an optional boolean.


28-41: LGTM! Props initialization looks good.

The disabled prop is properly initialized with a default value of false.


57-77: Excellent accessibility implementation!

The accessibilityProps function provides a robust implementation that:

  • Correctly handles both intrinsic buttons and custom elements
  • Properly sets aria-disabled attribute
  • Prevents click events when disabled
  • Includes helpful documentation

This is a great example of accessible component design.

frontend/src/components/Button/index.css.ts (1)

61-69: Verify color contrast for accessibility.

Please ensure the disabled state colors (Neutral 300 background with White text) meet WCAG contrast requirements for better accessibility.

❌ Verification inconclusive

🌐 Web query:

What is the minimum contrast ratio required by WCAG 2.1 for text?

💡 Result:

The minimum contrast ratio required by **WCAG 2.1 AA** for text depends on its size and weight:

| Text Type               | Minimum Contrast Ratio | Definition                                                                 |
|-------------------------|------------------------|----------------------------------------------------------------------------|
| **Normal Text**         | **4.5:1**              | Text smaller than **18pt (24px)** or **14pt bold (~18.66px)**              |
| **Large Text**          | **3:1**                | Text that is **≥18pt (24px)** or **≥14pt bold (~18.66px)**                 |
| **UI Components/Graphics** | **3:1**            | Non-text elements like icons, form borders, or graphical objects[5][9]. |

### Key Details:
- These requirements apply to both standard text and **images of text**[2][7].
- Exceptions include purely decorative elements, inactive UI components, and logos[7][9].
- For Level AAA compliance, stricter ratios apply (**7:1** for normal text and **4.5:1** for large text)[2][5].

### Examples of Compliant Contrast:
- **Normal Text**: Gray (#767676) on white[2].
- **Large Text**: Purple (#C86FF1) bolded at ≥14pt on white[2].

Tools like WebAIM’s Contrast Checker[5] can validate these ratios using hexadecimal color codes or a color picker.

Citations:

- 1: https://www.accessibilitychecker.org/wcag-guides/ensure-the-contrast-between-foreground-and-background-colors-meets-wcag-2-aa-minimum-contrast-ratio-thresholds/
- 2: https://webaim.org/articles/contrast/
- 3: https://www.digitala11y.com/understanding-sc-1-4-3-contrast-minimum/
- 4: https://www.a11y-collective.com/glossary/colour-contrast/
- 5: https://webaim.org/resources/contrastchecker/
- 6: https://my.willamette.edu/site/digital-accessibility/guidelines/color-contrast
- 7: https://www.w3.org/WAI/WCAG20/Understanding/contrast-minimum
- 8: https://docs.sourcepoint.com/hc/en-us/articles/4408054975891-WCAG-2-1-compliancy-custom-message
- 9: https://www.w3.org/WAI/WCAG22/Understanding/contrast-minimum.html
- 10: https://dequeuniversity.com/rules/axe/4.8/color-contrast

Action: Verify Disabled State Color Contrast Compliance

Based on WCAG 2.1 guidelines, text should have a contrast ratio of at least 4.5:1 for normal text (or 3:1 if classified as large text). Please review the actual computed values for the color tokens:

  • Verify the hexadecimal values for vars.color.Ref.Netural[300] (background) and vars.color.Ref.Netural['White'] (text).
  • Use a contrast checker (e.g., WebAIM’s Contrast Checker) to ensure the ratio meets at least 4.5:1 for normal text or 3:1 if the button text qualifies as large text.
  • Adjust the design tokens if the current combination does not meet the requirement.
frontend/src/routes/discussion/invite/$id.tsx (1)

6-18: LGTM! Clean implementation of the discussion invite route.

The route implementation follows best practices:

  • Uses the file-based routing pattern
  • Properly structures the layout with GlobalNavBar
  • Correctly implements the dynamic route parameter
frontend/src/pages/DiscussionInvitePage/index.tsx (1)

14-14: Add error handling for invalid data.

The component should handle cases where the data might be invalid or missing.

Consider adding error boundaries or validation:

-  return <DiscussionInviteCard {...dummyData} />;
+  return (
+    <ErrorBoundary fallback={<div>Something went wrong</div>}>
+      <DiscussionInviteCard {...dummyData} />
+    </ErrorBoundary>
+  );
frontend/src/features/discussion/ui/DiscussionInviteCard/index.css.ts (1)

1-41: LGTM! Well-structured styles using vanilla-extract.

The styles are well-organized, using appropriate vanilla-extract patterns:

  • Recipe for conditional styles
  • Theme variables for consistent spacing
  • Semantic class names
frontend/src/features/discussion/ui/DiscussionInviteCard/index.tsx (1)

44-44: Add error handling for participant images

The Avatar component should handle failed image loads gracefully.

❌ Verification inconclusive

🏁 Script executed:

#!/bin/bash
# Check if Avatar component has onError handling
ast-grep --pattern 'const Avatar = ({ $_, onError }) => {
  $$$
}'

Length of output: 68


Subject: Verification Needed: Avatar Component Error Handling Missing

The initial ast‑grep search did not reveal any evidence that the Avatar component is set up to handle image load errors (i.e. it doesn’t destructure or utilize an onError prop). We need to manually verify the component’s implementation to ensure that failed image loads are handled gracefully. For example, check if the underlying tag in the Avatar component has an onError attribute or if fallback logic is implemented.

• Inspect the Avatar component’s file (likely in a shared components folder) for onError usage in either its props or its element.
• If no error handling exists, please add logic to gracefully handle failed image loads (e.g., using an onError callback to load a fallback image).

frontend/src/components/Icon/component/UserTwo.tsx (1)

4-17: LGTM! Well-structured icon component.

The component follows React best practices with proper prop defaults, SVG attributes, and accessibility considerations.

frontend/src/components/Icon/component/CalendarCheck.tsx (1)

4-16: LGTM! Consistent icon implementation.

The component maintains consistency with other icon components while implementing proper SVG attributes and accessibility.

frontend/src/routeTree.gen.ts (1)

1-240: Skip review of auto-generated file.

This file is automatically generated by TanStack Router and should not be manually modified.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add unit to height value.

The height property is defined as a number without a unit, which may cause unexpected behavior. Add 'px' unit for consistency.

-  height: 32,
+  height: '32px',
📝 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.

Suggested change
height: 32,
height: '32px',

Comment on lines +10 to +13
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation for date parameter.

The function should validate that the input date is not null or undefined.

Apply this diff to add validation:

-export const getTimeParts = (date: Date) => ({
+export const getTimeParts = (date: Date) => {
+  if (!date) {
+    throw new Error('Date parameter is required');
+  }
+  return {
   hour: date.getHours(),
   minute: date.getMinutes(),
-});
+  };
+};
📝 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.

Suggested change
export const getTimeParts = (date: Date) => ({
hour: date.getHours(),
minute: date.getMinutes(),
});
export const getTimeParts = (date: Date) => {
if (!date) {
throw new Error('Date parameter is required');
}
return {
hour: date.getHours(),
minute: date.getMinutes(),
};
};

Comment on lines 23 to 26
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add ARIA attributes for better accessibility.

The badge should have appropriate ARIA attributes to improve accessibility.

Apply this diff:

-  <div className={badgeStyle}>
+  <div 
+    className={badgeStyle}
+    role="status"
+    aria-label={`${iconType} badge: ${children}`}
+  >
     {getIcon(iconType)}
     <Text typo='b3R'>{children}</Text>
   </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.

Suggested change
<div className={badgeStyle}>
{getIcon(iconType)}
<Text typo='b3R'>{children}</Text>
</div>
<div
className={badgeStyle}
role="status"
aria-label={`${iconType} badge: ${children}`}
>
{getIcon(iconType)}
<Text typo='b3R'>{children}</Text>
</div>

Comment on lines 7 to 9
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use unique mask ID to prevent conflicts

The mask ID should be unique to prevent conflicts when multiple instances of the component are rendered.

-<mask id="mask0_732_837" mask-type='luminance' maskUnits="userSpaceOnUse" x="0" y="0" width="24" height="24">
+<mask id={`clock-mask-${width}`} mask-type='luminance' maskUnits="userSpaceOnUse" x="0" y="0" width="24" height="24">

And update the mask reference:

-<g mask="url(#mask0_732_837)">
+<g mask={`url(#clock-mask-${width})`}>

Comment on lines 6 to 13
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Apply same SVG improvements as Clock component

The PinLocation component has the same SVG-related improvements needed as the Clock component:

  1. Add preserveAspectRatio and role
  2. Use unique mask ID
-        <svg width={width} height={width} viewBox="0 0 24 24"  xmlns="http://www.w3.org/2000/svg" aria-label="pin-location icon" fill="none" className={className} style={{ cursor: clickable ? "pointer": "default", ...rest.style }} {...rest}>
+        <svg width={width} height={width} viewBox="0 0 24 24" preserveAspectRatio="xMidYMid meet" xmlns="http://www.w3.org/2000/svg" aria-label="pin-location icon" role="img" fill="none" className={className} style={{ cursor: clickable ? "pointer": "default", ...rest.style }} {...rest}>
-<mask id="mask0_732_606" mask-type='luminance' maskUnits="userSpaceOnUse" x="0" y="0" width="24" height="24">
+<mask id={`pin-location-mask-${width}`} mask-type='luminance' maskUnits="userSpaceOnUse" x="0" y="0" width="24" height="24">
 <rect width="24" height="24" fill="white"/>
 </mask>
-<g mask="url(#mask0_732_606)">
+<g mask={`url(#pin-location-mask-${width})`}>
📝 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.

Suggested change
<svg width={width} height={width} viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg" aria-label="pin-location icon" fill="none" className={className} style={{ cursor: clickable ? "pointer": "default", ...rest.style }} {...rest}>
<mask id="mask0_732_606" mask-type='luminance' maskUnits="userSpaceOnUse" x="0" y="0" width="24" height="24">
<rect width="24" height="24" fill="white"/>
</mask>
<g mask="url(#mask0_732_606)">
<path fillRule="evenodd" clipRule="evenodd" d="M10.9455 12.5506C10.6102 12.4174 10.3049 12.2186 10.0473 11.9659C9.78976 11.7133 9.58516 11.4118 9.44549 11.0791C9.30583 10.7465 9.23389 10.3893 9.23389 10.0285C9.23389 9.66772 9.30583 9.31055 9.44549 8.97789C9.58516 8.64523 9.78976 8.34375 10.0473 8.09109C10.3049 7.83843 10.6102 7.63967 10.9455 7.50641C11.2808 7.37315 11.6393 7.30809 12 7.31501C12.7106 7.32865 13.3876 7.62054 13.8853 8.12793C14.3831 8.63532 14.6619 9.31773 14.6619 10.0285C14.6619 10.7393 14.3831 11.4217 13.8853 11.9291C13.3876 12.4365 12.7106 12.7284 12 12.742C11.6393 12.7489 11.2808 12.6839 10.9455 12.5506ZM9.87599 1.03701C5.60599 1.99001 2.61198 6.00901 2.77998 10.381C2.91198 13.815 5.01898 16.887 11.288 22.914C11.684 23.294 12.318 23.296 12.715 22.915C19.199 16.682 21.231 13.61 21.231 10.028C21.231 4.23801 15.898 0.306989 9.87599 1.03801" fill={fill}/>
</g>
</svg>
<svg width={width} height={width} viewBox="0 0 24 24" preserveAspectRatio="xMidYMid meet" xmlns="http://www.w3.org/2000/svg" aria-label="pin-location icon" role="img" fill="none" className={className} style={{ cursor: clickable ? "pointer": "default", ...rest.style }} {...rest}>
<mask id={`pin-location-mask-${width}`} mask-type='luminance' maskUnits="userSpaceOnUse" x="0" y="0" width="24" height="24">
<rect width="24" height="24" fill="white"/>
</mask>
<g mask={`url(#pin-location-mask-${width})`}>
<path fillRule="evenodd" clipRule="evenodd" d="M10.9455 12.5506C10.6102 12.4174 10.3049 12.2186 10.0473 11.9659C9.78976 11.7133 9.58516 11.4118 9.44549 11.0791C9.30583 10.7465 9.23389 10.3893 9.23389 10.0285C9.23389 9.66772 9.30583 9.31055 9.44549 8.97789C9.58516 8.64523 9.78976 8.34375 10.0473 8.09109C10.3049 7.83843 10.6102 7.63967 10.9455 7.50641C11.2808 7.37315 11.6393 7.30809 12 7.31501C12.7106 7.32865 13.3876 7.62054 13.8853 8.12793C14.3831 8.63532 14.6619 9.31773 14.6619 10.0285C14.6619 10.7393 14.3831 11.4217 13.8853 11.9291C13.3876 12.4365 12.7106 12.7284 12 12.742C11.6393 12.7489 11.2808 12.6839 10.9455 12.5506ZM9.87599 1.03701C5.60599 1.99001 2.61198 6.00901 2.77998 10.381C2.91198 13.815 5.01898 16.887 11.288 22.914C11.684 23.294 12.318 23.296 12.715 22.915C19.199 16.682 21.231 13.61 21.231 10.028C21.231 4.23801 15.898 0.306989 9.87599 1.03801" fill={fill}/>
</g>
</svg>

Comment on lines +167 to +169
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix misleading variable name and logic.

The variable isSameYear is misleading as it's true when years are different. This makes the code harder to understand, even though it functionally works correctly.

Apply this diff to improve clarity:

-  const isSameYear = startY !== endY;
+  const shouldIncludeYear = startY !== endY;
   const format = (year: number, month: number, day: number): string =>
-    isSameYear ? `${year}년 ${month + 1}월 ${day}일` : `${month + 1}월 ${day}일`;
+    shouldIncludeYear ? `${year}년 ${month + 1}월 ${day}일` : `${month + 1}월 ${day}일`;
📝 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.

Suggested change
const isSameYear = startY !== endY;
const format = (year: number, month: number, day: number): string =>
isSameYear ? `${year}${month + 1}${day}일` : `${month + 1}${day}일`;
const shouldIncludeYear = startY !== endY;
const format = (year: number, month: number, day: number): string =>
shouldIncludeYear ? `${year}${month + 1}${day}일` : `${month + 1}${day}일`;

Copy link
Contributor

@hamo-o hamo-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굿굿 수고하셨어요

Comment on lines +3 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p2;
저도 앞선 PR에서 놓쳤던 부분인데, 이제 API 명세가 어느정도 나왔으니 참고해서 model 먼저 적어주시고 타입 활용해서 내부에 내려줘도 좋을 것 같습니다.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요거 그냥 제가 API 연결할때 바꿀게요!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just ask;
typeof Component === 'string' 는 어떤 역할을 하나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다시보니 아무 역할도 없군요 ,, 제거하겠습니다

@dioo1461 dioo1461 force-pushed the feature/fe/discussion-invite-page branch from a9ae3cd to aa20d3b Compare February 11, 2025 02:56
@dioo1461 dioo1461 merged commit db5ce9b into dev Feb 11, 2025
1 check was pending
@dioo1461 dioo1461 deleted the feature/fe/discussion-invite-page branch February 11, 2025 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🖥️ FE Frontend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants