[FE-Feat] 에러페이지 마크업, 버튼 컴포넌트 다형성 리팩터링 #111
Conversation
WalkthroughThis pull request standardizes the naming convention for button styling by renaming the "type" property to "variant" across multiple files. It updates the Button component’s API—including its stories, CSS recipes, and prop interfaces—as well as its usage in related components such as TimeControlButton, SegmentControl, and PopoverButton. Additionally, the routing logic now imports and uses an ErrorPage component for handling 404 routes. There are modifications to landing page styles and theme adjustments in font weights and global CSS resets, with a new font weight entry added. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as Router
participant E as ErrorPage
U->>R: Navigate to an invalid route
R-->>E: Render ErrorPage via notFoundComponent
E-->>U: Display error message and "Go Home" button
U->>R: Clicks button to redirect to home page
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (4)
frontend/src/routes/@components/Error/index.tsx (2)
25-26: Consider using an i18n solution for text content.The Korean text strings should be managed through an internationalization system for better maintainability and potential future localization.
15-19: Consider moving the image path to a constants file.The hardcoded image path could be moved to a constants file for better maintainability and reusability.
frontend/src/components/Button/index.tsx (1)
44-53: Consider adding accessibility attributes.While the polymorphic implementation is solid, consider adding role="button" when rendering as a non-button element to maintain accessibility.
<Component className={clsx(containerStyle({ variant, style, radius, size }), className)} onClick={onClick} + role={as && as !== 'button' ? 'button' : undefined} >frontend/src/components/Calendar/Core/TimeControlButton.tsx (1)
13-19: Consider using polymorphic Button component.Now that the Button component supports polymorphism, consider replacing the native button elements with the Button component for consistent styling and behavior.
- <button - aria-label='이전 주' - className={timeControlButtonStyle({ order: 'first' })} - onClick={handleClickPrevWeek} - > + <Button + aria-label='이전 주' + className={timeControlButtonStyle({ order: 'first' })} + onClick={handleClickPrevWeek} + variant='secondary' + style='borderless' + > <ChevronLeft clickable fill={vars.color.Ref.Netural[600]} /> - </button> + </Button>Also applies to: 23-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
frontend/src/components/Button/Button.stories.tsx(1 hunks)frontend/src/components/Button/index.css.ts(13 hunks)frontend/src/components/Button/index.tsx(3 hunks)frontend/src/components/Calendar/Core/TimeControlButton.tsx(1 hunks)frontend/src/components/SegmentControl/index.tsx(1 hunks)frontend/src/routes/@components/Error/index.tsx(1 hunks)frontend/src/routes/__root.tsx(2 hunks)frontend/src/routes/landing/@components/index.css.ts(2 hunks)frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverButton.tsx(1 hunks)frontend/src/theme/font.ts(3 hunks)frontend/src/theme/reset.css.ts(1 hunks)frontend/src/theme/typo.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/routes/__root.tsx
[error] 6-6: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
frontend/src/routes/@components/Error/index.tsx
[error] 8-8: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Send PR Notification to Reviewers
🔇 Additional comments (14)
frontend/src/theme/typo.ts (1)
15-15: LGTM! Good addition of a semantic name for bold weight.The addition of
bold: 700improves maintainability by providing a semantic name for the bold weight value, which is consistent with CSS standards.frontend/src/theme/font.ts (1)
16-16: LGTM! Good improvement to the typographic hierarchy.The font weight adjustments for headings create a clearer visual hierarchy:
- H1 and H2 now use bold weight (700)
- H3 uses semi-bold weight (600)
This aligns well with common typographic practices where primary headings have stronger emphasis.
Also applies to: 27-27, 38-38
frontend/src/routes/__root.tsx (1)
24-24: LGTM! Proper integration of error handling.The
notFoundComponentis correctly configured to handle 404 scenarios using TanStack Router's API.frontend/src/routes/@components/Error/index.tsx (1)
28-35: Great implementation of the polymorphic Button component!The Button component is correctly used with the polymorphic 'as' prop to integrate with the Link component, demonstrating the successful implementation of the button component's polymorphic capabilities as mentioned in the PR objectives.
frontend/src/components/Button/index.tsx (2)
14-16: LGTM! Well-designed polymorphic type.The
AsProptype is well-designed with proper generic constraints and default type parameter.
30-41: LGTM! Excellent polymorphic component implementation.The component signature correctly combines
AsProp<T>,ComponentPropsWithoutRef<T>, andButtonPropswhile maintaining type safety.frontend/src/components/Button/Button.stories.tsx (1)
10-13: LGTM! Consistent prop renaming in Storybook configuration.The argTypes correctly reflect the Button component's prop rename from 'type' to 'variant' while maintaining the same options.
frontend/src/components/Calendar/Core/TimeControlButton.tsx (1)
37-37: LGTM! Consistent prop update.The Button prop has been correctly updated from 'type' to 'variant'.
frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverButton.tsx (1)
24-24: LGTM! Consistent prop update.The Button prop has been correctly updated from 'type' to 'variant'.
frontend/src/components/SegmentControl/index.tsx (1)
39-39: LGTM: Button prop renamed correctlyThe change from
typetovariantaligns with the Button component's interface update.frontend/src/theme/reset.css.ts (1)
23-34: Verify layout integrity after positioning changesThe changes to html/body positioning from
fixedtoabsoluteand removal of#rootand#pageselectors could affect the application's layout structure. Please ensure:
- No scrolling issues occur in different viewport sizes
- Layout remains stable across different routes
- No unintended overflow occurs
frontend/src/routes/landing/@components/index.css.ts (1)
8-18: LGTM: Responsive overflow handlingThe overflow handling with media query is well-implemented for responsive design:
- Hidden overflow by default
- Scrollable on smaller screens (≤1024px)
frontend/src/components/Button/index.css.ts (2)
13-26: Clarify the purpose of 're' variantWhile the renaming from
typetovariantis correct, therevariant name is not descriptive and deviates from the naming pattern of other variants (primary, secondary, destructive).Could you clarify the purpose of the
revariant? Consider using a more descriptive name that follows the established pattern.
63-182: LGTM: Consistent variant renamingThe renaming from
typetovariantis consistently applied across all variant definitions, compound variants, and default variants.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
frontend/src/components/Button/index.css.ts (2)
23-25: Consider removing redundant 're' variant.The 're' variant appears to be a duplicate of the 'destructive' variant as they both use the same color token
vars.color.Ref.Red[500]. Consider consolidating these variants to maintain a cleaner API.- re: { - color: vars.color.Ref.Red[500], - },
146-163: Remove redundant compound variants for 're' variant.These compound variants for the 're' variant are redundant with the 'destructive' variant's styles. Consider removing them to maintain consistency and reduce code duplication.
- { - variants: { - variant: 're', - style: 'filled', - }, - style: { - backgroundColor: vars.color.Ref.Red[500], - }, - }, - { - variants: { - variant: 're', - style: 'outline', - }, - style: { - backgroundColor: vars.color.Ref.Netural.White, - borderColor: vars.color.Ref.Red[500], - }, - },
🧹 Nitpick comments (4)
frontend/src/theme/reset.css.ts (1)
23-23: Consider documenting the selector change.The removal of
#rootand#pagefrom the selector makes the styles more focused on standard elements, which is good. However, please document the reason for this change to help future maintainers understand the decision.frontend/src/components/Button/index.css.ts (3)
18-18: Fix typo in color token name.There appears to be a typo in the color token name: 'Netural' should be 'Neutral'.
- color: vars.color.Ref.Netural[700], + color: vars.color.Ref.Neutral[700], - backgroundColor: vars.color.Ref.Netural[700], + backgroundColor: vars.color.Ref.Neutral[700], - backgroundColor: vars.color.Ref.Netural[100], + backgroundColor: vars.color.Ref.Neutral[100], - backgroundColor: vars.color.Ref.Netural.White, + backgroundColor: vars.color.Ref.Neutral.White, - color: vars.color.Ref.Netural[500], + color: vars.color.Ref.Neutral[500],Also applies to: 96-96, 105-105, 114-114, 124-124
6-11: Enhance base styles with accessibility properties.Consider adding the following accessibility-related properties to the base styles:
base: { display: 'flex', alignItems: 'center', gap: vars.spacing[200], cursor: 'pointer', + userSelect: 'none', + ':focus-visible': { + outline: `2px solid ${vars.color.Ref.Primary[500]}`, + outlineOffset: '2px', + }, },
165-175: Consider extracting small size styles into a separate variant.The small size variant overrides multiple properties and sets a distinct visual style. Consider extracting these styles into a separate style variant for better organization and maintainability.
variants: { style: { weak: {}, filled: { color: vars.color.Ref.Neutral.White, }, outline: { borderWidth: 1, borderStyle: 'solid', }, borderless: {}, + minimal: { + height: 18, + gap: vars.spacing[100], + color: vars.color.Ref.Neutral[500], + backgroundColor: 'transparent', + border: 'none', + }, },Then update the compound variant:
- { - variants: { - size: 'sm', - }, - style: { - height: 18, - gap: vars.spacing[100], - color: vars.color.Ref.Netural[500], - backgroundColor: 'transparent', - border: 'none', - }, - }, + { + variants: { + size: 'sm', + }, + style: { + style: 'minimal', + }, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
frontend/src/components/Button/Button.stories.tsx(1 hunks)frontend/src/components/Button/index.css.ts(13 hunks)frontend/src/components/Button/index.tsx(3 hunks)frontend/src/components/Calendar/Core/TimeControlButton.tsx(1 hunks)frontend/src/components/SegmentControl/index.tsx(1 hunks)frontend/src/routes/@components/Error/index.tsx(1 hunks)frontend/src/routes/__root.tsx(2 hunks)frontend/src/routes/landing/@components/index.css.ts(2 hunks)frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverButton.tsx(1 hunks)frontend/src/theme/font.ts(3 hunks)frontend/src/theme/reset.css.ts(1 hunks)frontend/src/theme/typo.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/routes/__root.tsx
[error] 6-6: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
frontend/src/routes/@components/Error/index.tsx
[error] 8-8: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Send PR Notification to Reviewers
🔇 Additional comments (20)
frontend/src/theme/typo.ts (1)
15-15: LGTM! Good addition of a semantic font weight.The addition of
bold: 700provides a semantic way to reference bold font weight, which is more maintainable than using numeric values directly.frontend/src/theme/font.ts (1)
16-16: LGTM! Good improvement to heading hierarchy.The font weight changes create a clearer visual hierarchy:
- H1 and H2 now use the semantic bold weight (700)
- H3 uses pretendard-0 (600)
This follows the common pattern where higher-level headings have stronger visual emphasis.
Also applies to: 27-27, 38-38
frontend/src/routes/__root.tsx (1)
24-24: LGTM! Good implementation of 404 handling.The integration of the error component as
notFoundComponentis a clean way to handle 404 cases.frontend/src/routes/@components/Error/index.tsx (1)
28-35: Great use of polymorphic Button component!The implementation perfectly demonstrates the button's polymorphic capabilities by using it as a Link component. This eliminates the need for wrapper components or click handlers.
frontend/src/components/Button/index.tsx (7)
1-6: Imports look correct.
All necessary types are imported from React, ensuring type safety for polymorphic component usage.
14-16: Definition of AsProp is well-structured.
Using a generic type parameter with a default of 'button' is a good approach for polymorphism.
19-19: Renaming 'type' to 'variant' is clear and consistent.
This aligns well with the new naming convention introduced throughout the codebase.
30-32: Polymorphic Button component signature is well-defined.
Specifying a default element type and destructuring props ensures flexibility and reusability.
41-43: Generic intersection of props is correct.
Combining AsProp, ComponentPropsWithoutRef, and ButtonProps accurately covers various element attributes.
44-53: Rendering logic is clear.
The default fallback to 'button' is appropriate, and clsx usage is concise for conditional classes.
54-54: Export statement is straightforward.
No issues with exporting the Button component as the default.frontend/src/routes/my-schedule/@components/SchedulePopover/PopoverButton.tsx (1)
24-24: Renaming to 'variant' ensures consistency.
Matches the revised property name in the Button component while preserving the 'destructive' styling intent.frontend/src/components/Button/Button.stories.tsx (1)
10-10: Storybook argTypes updated successfully.
The switch from 'type' to 'variant' aligns with the main Button component, ensuring accurate variant selection in stories.frontend/src/components/Calendar/Core/TimeControlButton.tsx (1)
37-37: LGTM! Prop name change aligns with the new Button API.The change from
typetovariantis consistent with the Button component's API update.frontend/src/components/SegmentControl/index.tsx (1)
39-39: LGTM! Consistent prop name update.The change from
typetovariantin buttonStyles maintains consistency with the updated Button API.frontend/src/theme/reset.css.ts (1)
29-29: Verify layout behavior with position: absolute.Changing position from
fixedtoabsolutecould affect the page's layout and scrolling behavior. Please ensure this change doesn't cause unintended side effects across different viewport sizes and content lengths.Run this script to find potential layout issues:
frontend/src/routes/landing/@components/index.css.ts (3)
8-8: Verify fixed height impact on content.Using a fixed
height: 100vhinstead ofminHeightmight cause content clipping issues when content exceeds viewport height.Consider testing with various content lengths to ensure no content is cut off.
12-18: LGTM! Good responsive overflow handling.The overflow handling is well implemented with responsive behavior for different screen sizes:
- Hidden overflow by default
- Scrollable on smaller screens (≤1024px)
73-74: Verify image size calculations.The new width calculation
calc(50%-${vars.spacing[300]})and reducedmaxHeightmight affect image display.Run this script to check for similar image sizing patterns:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for similar image size calculations rg "calc\(50%.*\)" --type css --type typescriptLength of output: 84
🏁 Script executed:
#!/bin/bash # Search for image size calculation patterns in .css.ts files echo "Searching for 'calc(50%-' patterns in .css.ts files:" rg "calc\(50%-" -g "*.css.ts" echo "" echo "Searching for 'maxHeight: '28rem'' settings in .css.ts files:" rg "maxHeight:\s*'28rem'" -g "*.css.ts"Length of output: 527
Final Image Size Calculation Verification
The search confirmed that the new width and max-height settings appear exclusively in
• frontend/src/routes/landing/@components/index.css.ts
with the following lines:
width:calc(50%-${vars.spacing[300]})
maxHeight: '28rem'No similar image sizing calculations were found elsewhere in the codebase. Please ensure that the missing spaces in the calc expression (i.e., “50%-”) are intentional and render correctly in your target browsers.
frontend/src/components/Button/index.css.ts (1)
177-182: LGTM! Default variants are well-configured.The default variant configuration provides a good starting point with primary filled buttons of medium size and maximum radius.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/routes/@components/ErrorPage/index.tsx (1)
12-13: Consider using theme-based spacing values.The gap value of 700 appears to be a magic number. Consider using theme-based spacing variables for consistency with the rest of the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/routes/@components/ErrorPage/index.tsx(1 hunks)frontend/src/routes/__root.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/__root.tsx
🔇 Additional comments (1)
frontend/src/routes/@components/ErrorPage/index.tsx (1)
28-35: Great implementation of polymorphic Button!The Button component is well implemented using the new polymorphic capabilities, allowing it to function as a Link component. This is exactly what was intended in the PR objectives.
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
2025-02-07.4.54.17.mov
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Link할 때 쓰게 될 것 같네요. 굳이 다른 컴포넌트로 감싸거나 불필요한 클릭 이벤트를 달지 않아도 되어 편리합니다.Summary by CodeRabbit
New Features
Style
Bug Fixes
typetovariantacross various components for consistent styling.