[FE-Feat] 랜딩페이지 마크업 및 이미지 webp 변환 스크립트 구현 #103
Conversation
WalkthroughThis pull request introduces several enhancements across the frontend. It updates the routing system by adding a new landing page route and associated components and styles, while removing an outdated lazy-loaded home route. New UI components, including a Google icon and a responsive image component, are added. A minor update to the Button component reorders arguments for class names. A new script using the "sharp" library automates image conversion and optimization, with related changes added to package.json. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as Root Route ("/")
participant L as Landing Route ("/landing")
U->>R: Access "/"
R->>R: Trigger beforeLoad hook
R->>L: Redirect to "/landing"
L-->>U: Render Landing page
Assessment against linked issues
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 (
|
hamo-o
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
frontend/src/scripts/optimize-image.cjs (1)
1-10: Consider using TypeScript for better type safety.The script setup looks good, but converting it to TypeScript would provide better type safety and improved IDE support.
-const fs = require("fs"); -const path = require("path"); -const sharp = require("sharp"); +import * as fs from 'fs'; +import * as path from 'path'; +import sharp from 'sharp'; +interface Breakpoint { + width: number; + suffix: string; +} -const BREAKPOINTS = [ +const BREAKPOINTS: Breakpoint[] = [frontend/src/components/Text/index.tsx (1)
3-4: LGTM! Consider documenting the className prop.The
classNameprop addition andclsxusage are well implemented. Consider adding JSDoc comments to document the purpose and usage of theclassNameprop for better developer experience.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/routes/landing/@components/LandingImages.tsx (1)
1-18: LGTM! Clean implementation with good accessibility.The component follows React best practices, uses descriptive alt text for accessibility, and implements webp format as requested.
Consider adding loading state handling to improve user experience:
export const LandingImages = () => ( <div className={imageWrapperStyle}> <Image alt='구글 캘린더 연동으로 간편하고 빠른 일정입력' className={imageStyle} + isLazy={true} src='/images/landing/landing-1.webp' /> <Image alt='참여자들의 일정을 고려한 추천으로 간단하게 끝나는 일정 조율' className={imageStyle} + isLazy={true} src='/images/landing/landing-2.webp' /> </div> );frontend/src/components/Image/index.tsx (1)
8-14: Move breakpoints to theme configuration.Consider moving the breakpoints to the theme configuration for better maintainability and consistency across components.
-const BREAKPOINTS = [480, 768, 1280]; -const SIZES = ` -(max-width: 480px) 480px, -(max-width: 768px) 768px, -(max-width: 1280px) 1280px, -(min-width: 1281px) 1920px -`; +import { vars } from '@/theme/index.css'; + +const BREAKPOINTS = vars.breakpoints; +const SIZES = Object.entries(BREAKPOINTS) + .map(([key, value]) => `(max-width: ${value}px) ${value}px`) + .join(',\n') + ',\n(min-width: 1281px) 1920px';frontend/src/routes/landing/@components/index.tsx (2)
22-44: Consider adding i18n support.The text content is currently hardcoded in Korean. Consider using an i18n library for better maintainability and future localization support.
Example using react-i18next:
+import { useTranslation } from 'react-i18next'; const Landing = () => { + const { t } = useTranslation(); const handleClickGoogleLogin = () => { // do something }; return ( <div className={containerStyle}> <h1 className={titleStyle}> - 언제 만나? + {t('landing.title')} </h1> <h2 className={subTitleStyle}> - 더이상 고민하지 마세요! + {t('landing.subtitle')} </h2> <Text className={descriptionStyle} color={vars.color.Ref.Netural[700]} typo='b2M' > - 당신과 모두의 일정을 하나로 연결해 가장 완벽한 약속 시간을 찾아드려요 + {t('landing.description.line1')} <br /> - 당신과 모두의 시간을 위해, 지금 바로 시작하세요. + {t('landing.description.line2')} </Text> <Button className={buttonStyle} leftIcon={<Google />} onClick={handleClickGoogleLogin} size='xl' > - Google로 시작하기 + {t('landing.googleButton')} </Button>
29-31: Create color constant for better maintainability.Consider creating a semantic color constant instead of directly using the color value from theme vars.
+const DESCRIPTION_COLOR = vars.color.Ref.Netural[700]; + const Landing = () => { return ( // ... <Text className={descriptionStyle} - color={vars.color.Ref.Netural[700]} + color={DESCRIPTION_COLOR} typo='b2M' >frontend/src/components/Icon/component/Google.tsx (1)
2-2: Consider using more specific prop types.Instead of spreading
...rest, consider explicitly defining the SVG attributes you expect to receive for better type safety.-import type { IconProps } from '../Icon.d.ts'; +import type { IconProps } from '../Icon.d.ts'; +import type { SVGAttributes } from 'react'; -export const Google = ({ clickable = false, className, width = 24 , fill = "white", ...rest }: IconProps) => { +export const Google = ({ + clickable = false, + className, + width = 24, + fill = "white", + ...rest +}: IconProps & Omit<SVGAttributes<SVGElement>, keyof IconProps>) => {Also applies to: 4-4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 49606ed and 20d6f4b9c2c58566ba062a7391c74f2a7ead1946.
⛔ Files ignored due to path filters (2)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/src/components/Icon/svg/google.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
frontend/package.json(2 hunks)frontend/src/components/Button/index.tsx(3 hunks)frontend/src/components/Icon/component/Google.tsx(1 hunks)frontend/src/components/Icon/index.ts(1 hunks)frontend/src/components/Image/index.tsx(1 hunks)frontend/src/components/Text/index.tsx(1 hunks)frontend/src/routeTree.gen.ts(5 hunks)frontend/src/routes/index.lazy.tsx(0 hunks)frontend/src/routes/index.tsx(1 hunks)frontend/src/routes/landing/@components/LandingImages.tsx(1 hunks)frontend/src/routes/landing/@components/index.css.ts(1 hunks)frontend/src/routes/landing/@components/index.tsx(1 hunks)frontend/src/routes/landing/index.tsx(1 hunks)frontend/src/scripts/optimize-image.cjs(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/routes/index.lazy.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Send PR Notification to Reviewers
🔇 Additional comments (9)
frontend/package.json (1)
16-16: LGTM! Script and dependency look good.The script name is descriptive and the sharp version is up to date.
Also applies to: 58-58
frontend/src/routes/landing/index.tsx (1)
1-7: LGTM! Clean and standard route setup.The route is correctly configured using
@tanstack/react-router'screateFileRoutewith proper component association.frontend/src/routes/index.tsx (1)
1-9: LGTM! Clean root route with proper redirection.The root route is correctly configured to redirect to the landing page using
@tanstack/react-router'sbeforeLoadhook.frontend/src/components/Icon/index.ts (1)
7-7: LGTM! Verify the Google icon implementation.The export follows the consistent pattern used for other icon components.
Please ensure that the Google icon component in
./component/Google.tsx:
- Follows the same implementation pattern as other icon components
- Includes proper SVG attributes for accessibility
- Has appropriate default props for width, height, and colors
Run this script to verify:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the Google icon component follows the same pattern as other icons ast-grep --pattern $'const Google = ({ clickable, className, width, fill, ...rest }: IconProps) => { $$$ return ( <svg $$$ aria-hidden="true" $$$ > $$$ </svg> ); };' frontend/src/components/Icon/component/Google.tsxLength of output: 258
🏁 Script executed:
#!/bin/bash echo "=== Checking for 'Google' component definition in Google.tsx ===" rg "const Google" frontend/src/components/Icon/component/Google.tsx echo "" echo "=== Checking for SVG accessibility attribute 'aria-hidden' in Google.tsx ===" rg "aria-hidden=" frontend/src/components/Icon/component/Google.tsx echo "" echo "=== Checking for default props definitions in Google.tsx ===" rg "defaultProps" frontend/src/components/Icon/component/Google.tsxLength of output: 783
🏁 Script executed:
#!/bin/bash # Extract and show the <svg> element and its attributes from the Google icon file for manual inspection. rg "<svg" frontend/src/components/Icon/component/Google.tsx -n -C 10Length of output: 1684
Google Icon Implementation Verified
- The component uses inline default props (
clickable,width,fill) consistent with other icon components.- The
<svg>element includes accessibility attributes viaaria-label="google icon", which provides a clear accessible name.- The implementation pattern (export, component definition, and inline defaults) is consistent with the other icons.
frontend/src/components/Image/index.tsx (1)
16-33: Add error handling and document file naming convention.The component assumes specific file naming convention for responsive images but lacks error handling.
- Document the required file naming convention in the component's JSDoc:
+/** + * Image component with responsive image support. + * Required file naming convention for responsive images: + * - Base file: image.webp + * - Responsive files: image-480w.webp, image-768w.webp, image-1280w.webp + */ export const Image = ({ src, alt, isLazy = false, className }: ImageProps) => {
- Add error handling for missing responsive images:
export const Image = ({ src, alt, isLazy = false, className }: ImageProps) => { const BASE_PATH = src.split('.')[0]; + const [basePath, extension] = src.split('.'); + + if (extension !== 'webp') { + console.warn(`Image ${src} should use webp format for better performance`); + } + const createSrcSet = () => BREAKPOINTS .map((width) => `${BASE_PATH}-${width}w.webp ${width}w`) .join(', '); return ( <img alt={alt} className={className} loading={isLazy ? 'lazy' : 'eager'} sizes={SIZES} src={src} srcSet={`${createSrcSet()}, ${src} 1920w`} + onError={(e) => { + // Fallback to original src if responsive images fail to load + e.currentTarget.srcset = ''; + }} /> ); };✅ Verification successful
Action Required: Documentation and Error Handling Updated in Image Component
The revisions in the component now include JSDoc comments that outline the required file naming convention for responsive images, and error handling has been added to warn developers if the image extension isn’t in the expected webp format, as well as to provide a fallback via the onError handler.
- JSDoc documents the required file naming convention.
- A warning is issued if the image file extension isn’t “webp”.
- An onError handler is provided to remove the srcSet fallback if responsive images fail to load.
frontend/src/components/Button/index.tsx (1)
18-19: LGTM! Clean implementation of className prop.The changes correctly implement the optional className prop with proper TypeScript typing and class name composition using clsx.
Also applies to: 30-35
frontend/src/components/Icon/component/Google.tsx (1)
6-11: LGTM! Well-implemented SVG with accessibility.The SVG implementation is clean with proper accessibility attributes and dynamic styling.
frontend/src/routes/landing/@components/index.css.ts (1)
65-77: Verify image optimization for responsive sizes.Based on the PR objectives emphasizing image optimization, ensure that the images are being served in
webpformat and with appropriate sizes for different viewports.frontend/src/routeTree.gen.ts (1)
1-142: LGTM! Auto-generated route configuration.This is an auto-generated file by TanStack Router. The route configuration looks correct with proper type definitions and relationships.
hamo-o
force-pushed
the
feature/fe/landing-markup
branch
from
8fc73da to
fda8dd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/routes/landing/@components/index.css.ts (4)
37-43: Remove redundant display property.The
display: 'block'property is unnecessary since text-align works on block elements by default.export const descriptionStyle = style({ paddingTop: '0.625rem', paddingBottom: '3.375rem', - display: 'block', textAlign: 'center', });
45-47: Fix button centering approach.The
justifySelfproperty only works within a grid layout. For standalone centering, consider using margin or flexbox.export const buttonStyle = style({ - justifySelf: 'center', + display: 'block', + margin: '0 auto', });
49-63: Maintain consistent horizontal padding.The horizontal padding is removed in mobile view which might cause content to touch screen edges. Consider maintaining consistent horizontal padding across breakpoints.
export const imageWrapperStyle = style({ padding: '3rem 1.75rem 0', display: 'flex', alignItems: 'center', justifyContent: 'center', gap: vars.spacing[700], '@media': { '(max-width: 1024px)': { flexDirection: 'column', - padding: '3rem 0', + padding: '3rem 1.75rem', }, }, });
65-77: Optimize image height for mobile devices.Increasing maxHeight on mobile devices might lead to unnecessarily large images and potential performance issues. Consider reducing or maintaining the same maxHeight for better mobile optimization.
export const imageStyle = style({ width: '50%', maxHeight: '29.8rem', objectFit: 'cover', '@media': { '(max-width: 1024px)': { width: '100%', - maxHeight: '40rem', + maxHeight: '29.8rem', }, }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 20d6f4b9c2c58566ba062a7391c74f2a7ead1946 and fda8dd5.
⛔ Files ignored due to path filters (2)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlfrontend/src/components/Icon/svg/google.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
frontend/package.json(2 hunks)frontend/src/components/Button/index.tsx(1 hunks)frontend/src/components/Icon/component/Google.tsx(1 hunks)frontend/src/components/Icon/index.ts(1 hunks)frontend/src/components/Image/index.tsx(1 hunks)frontend/src/routeTree.gen.ts(5 hunks)frontend/src/routes/index.lazy.tsx(0 hunks)frontend/src/routes/index.tsx(1 hunks)frontend/src/routes/landing/@components/LandingImages.tsx(1 hunks)frontend/src/routes/landing/@components/index.css.ts(1 hunks)frontend/src/routes/landing/@components/index.tsx(1 hunks)frontend/src/routes/landing/index.tsx(1 hunks)frontend/src/scripts/optimize-image.cjs(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/routes/index.lazy.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- frontend/src/components/Icon/index.ts
- frontend/src/routes/landing/@components/LandingImages.tsx
- frontend/src/routes/index.tsx
- frontend/src/routes/landing/index.tsx
- frontend/src/components/Button/index.tsx
- frontend/package.json
- frontend/src/components/Icon/component/Google.tsx
- frontend/src/routes/landing/@components/index.tsx
- frontend/src/scripts/optimize-image.cjs
- frontend/src/components/Image/index.tsx
🔇 Additional comments (7)
frontend/src/routes/landing/@components/index.css.ts (3)
6-12: Fix potential horizontal overflow.Using
100vwcan cause horizontal scrolling due to the scrollbar width. Consider using100%instead.
14-26: Verify font sizes for mobile devices.The font size of 3.25rem (52px) might be too large for mobile screens. Consider adding responsive font sizes.
28-35: Fix typo in color token name.There's a typo in the color token name:
Neturalshould beNeutral.Verify font sizes for mobile devices.
The font size of 3.25rem (52px) might be too large for mobile screens. Consider adding responsive font sizes.
frontend/src/routeTree.gen.ts (4)
1-9:⚠️ Warning: This is an auto-generated file.The file header explicitly states that this is automatically generated by TanStack Router and should not be modified manually. Any direct changes to this file will be overwritten. Instead, update your route configurations in their respective source files.
47-70: Type declarations look good!The type declarations for the landing route are properly integrated across all necessary interfaces, maintaining type safety and consistency.
Also applies to: 75-101
119-141: Route manifest is properly updated.The route manifest correctly includes the landing route with the appropriate file path.
15-17: Consider performance impact of removing lazy loading.The IndexRoute has been changed from lazy loading to direct import. While this simplifies the code, it might impact initial load performance. Consider keeping lazy loading if the index route contains heavy components or is not frequently accessed.
Run this script to analyze the bundle size impact:
Also applies to: 25-29
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
webp확장자로 변환하고, 반응형에 대응하는 이미지를 만들어주는 스크립트를 작성했습니다. (거기에 필요한sharp패키지devDependencies로 설치했습니다.)webp로의 변환 꼭 부탁드려요~! (저는 처음에 안깨지게 하려고 4배로 뽑았다가 깃에 올라가지도 않음..)Summary by CodeRabbit