-
Notifications
You must be signed in to change notification settings - Fork 0
과목사전 페이지 + 계정 페이지 반응형 디자인 #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
MBP16
commented
Oct 31, 2025
- 과목사전 페이지에 새로 나온 디자인을 적용합니다
- 과목사전 페이지에 반응형 디자인을 적용합니다
- 계정 페이지에 반응형 디자인을 적용합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements responsive design improvements for tablet and mobile devices across the application. The changes introduce a new flexible device detection hook, refactor breakpoint types, and add mobile-specific UI behaviors including modals and collapsible sections.
Key changes include:
- Replacing
useIsMobilehook with a more flexibleuseIsDevicehook that supports multiple device types (mobile, tablet, laptop, desktop) - Adding responsive layouts with mobile/tablet breakpoints throughout the dictionary and account pages
- Implementing full-screen modals for mobile/tablet views
- Adding collapsible course history sections with fold/unfold functionality
- Refactoring API schemas to support the new course history data structure with multiple professors per class
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| app/utils/useIsDevice.ts | New hook replacing useIsMobile with support for multiple device types |
| app/utils/useIsMobile.ts | Removed old mobile-only detection hook |
| app/styles/themes/_base/variables/breakpoints.ts | Added DeviceType export and desktop breakpoint |
| app/styles/themes/media.ts | Updated to use DeviceType instead of local Breakpoint type |
| app/routes/dictionary.tsx | Added responsive layout with mobile modal and query parameter handling |
| app/features/dictionary/sections/CourseDetailSection/index.tsx | Added mobile modal support and scroll-to-review functionality |
| app/features/dictionary/sections/CourseDetailSection/CourseHistorySubsection.tsx | Added fold/unfold functionality and support for multiple professors per class |
| app/features/dictionary/sections/CourseDetailSection/CourseInfoSubsection.tsx | Updated to use new API field names and responsive layout |
| app/features/dictionary/sections/CourseDetailSection/CourseReviewSubsection.tsx | Improved responsive layout for review metrics |
| app/common/components/Modal.tsx | Added fullScreen prop for responsive modal behavior |
| app/common/components/search/SearchArea.tsx | Improved layout structure for better overflow handling |
| app/common/components/search/SearchFilterArea.tsx | Added scroll handling with hidden scrollbar |
| app/common/components/guideline/Header/index.tsx | Updated to use new useIsDevice hook |
| app/common/components/guideline/Header/Setting.tsx | Fixed language detection using resolvedLanguage |
| app/api/courses/$courseId.ts | Updated schema to support new course history structure |
| app/api/example/Course.tsx | Updated example data to match new API schema |
| app/globals.css | Changed min-height from 100vh to 100dvh for better mobile support |
| app/i18n/variables/en.ts | Added translation keys for "over" and "people" |
| app/i18n/variables/_base.ts | Added Korean translation keys |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| window.addEventListener("resize", handleResize) | ||
| return () => window.removeEventListener("resize", handleResize) | ||
| }, [type, theme.breakpoints]) |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect dependency array includes theme.breakpoints which is an object reference that will change on every render, causing unnecessary re-creation of event listeners. The getIsMatch function should also be included in the dependency array since it's used in the effect, or the function should be memoized. Consider extracting the specific breakpoint values or memoizing the getIsMatch function.
| } else if (selectedCourseId !== null) { | ||
| setMobileModal(true) | ||
| } | ||
| }, [isTablet]) |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect on line 71-77 is missing selectedCourseId in its dependency array. The condition on line 74 uses selectedCourseId !== null, but changes to this variable won't trigger the effect. This could cause the modal state to become out of sync when the selected course changes.
| }, [isTablet]) | |
| }, [isTablet, selectedCourseId]) |
| if (isTablet && selectedCourseId !== null) { | ||
| setMobileModal(true) | ||
| } | ||
| }, [selectedCourseId]) |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect on line 78-82 is missing isTablet in its dependency array. If the device type changes while a course is selected, the modal state won't update correctly. This could lead to inconsistent UI behavior when resizing the browser or rotating a device.
| }, [selectedCourseId]) | |
| }, [selectedCourseId, isTablet]) |
| courseDetail?.history?.some((history) => history.classes.length > 4) ?? false | ||
|
|
||
| setIsHistoryFolded(shouldFold) | ||
| }, [isMobile, courseDetail]) |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect dependency array is incomplete. The effect reads courseDetail?.history but only depends on courseDetail. If only the history array changes while the courseDetail object reference remains the same, the effect won't re-run. Additionally, the effect logic depends on the fold state calculation but doesn't trigger when needed.
| }, [isMobile, courseDetail]) | |
| }, [isMobile, courseDetail, courseDetail?.history]) |
|
|
||
| const SortWrapper = styled(FlexWrapper)` | ||
| width: 35%; | ||
| white-space: nowrap; |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The width: 35% style was removed from SortWrapper, but white-space: nowrap remains. Without a constrained width, nowrap may cause the sort controls to expand unexpectedly and overflow their container. Consider reviewing if this styling still makes sense or if flex properties should be adjusted.
| white-space: nowrap; | |
| white-space: nowrap; | |
| flex-shrink: 1; | |
| min-width: 0; |
| min-width: ${(props) => (props.fullScreen ? "100dvw" : "630px")}; | ||
| width: ${(props) => (props.fullScreen ? "100dvw" : "auto")}; | ||
| height: ${(props) => (props.fullScreen ? "100dvh" : "auto")}; | ||
| padding: 16px; |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When fullScreen is true, both width and height use dvh/dvw units (100dvh, 100dvw) along with border-radius: 0px and padding: 16px. This creates a modal that is larger than the viewport (100dvw + 32px horizontal padding). Consider using box-sizing: border-box or adjusting the dimensions to account for padding.
| padding: 16px; | |
| padding: 16px; | |
| box-sizing: border-box; |
|
|
||
| const [reviews, setReviews] = useState<GETReviewsResponse | null>(exampleReviews) | ||
| const [reviewLanguage, setReviewLanguage] = useState("all") | ||
| const [reviews, setReviews] = useState<GETReviewsResponse | null>(exampleReviews) |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable setReviews.
| const [reviews, setReviews] = useState<GETReviewsResponse | null>(exampleReviews) | |
| const reviews = useState<GETReviewsResponse | null>(exampleReviews)[0] |
| const theme = useTheme() | ||
|
|
||
| const [searchResult, setSearchResult] = useState<GETCoursesResponse | null>( | ||
| const [searchResult, setSearchResult] = useState<GETCoursesResponse>( |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable setSearchResult.
| const [searchResult, setSearchResult] = useState<GETCoursesResponse>( | |
| const [searchResult] = useState<GETCoursesResponse>( |
…ctionary # Conflicts: # app/api/courses/$courseId.ts # app/api/example/Course.tsx # app/features/dictionary/components/ReviewWritingBlock.tsx # app/features/dictionary/sections/CourseDetailSection/CourseHistorySubsection.tsx # app/features/dictionary/sections/CourseDetailSection/CourseInfoSubsection.tsx # app/features/dictionary/sections/CourseDetailSection/CourseReviewSubsection.tsx # app/features/dictionary/sections/CourseDetailSection/index.tsx # app/routes/dictionary.tsx