히스토리 페이지 개발#84
Conversation
Summary of ChangesHello @HWAN0218, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 히스토리 페이지의 전반적인 구조를 리팩토링하고 기능을 개선합니다. 페이지 경로를 변경하고, 불필요한 UI 컴포넌트 의존성을 제거하며, 완료된 작업 데이터를 보다 효율적으로 처리하도록 로직을 업데이트했습니다. 또한, 팀 관리 기능을 추가하고 사용자 경험을 위한 UI/UX 조정을 포함합니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the MyHistory component, primarily by removing the desktop sidebar and mobile GNB UI elements and their associated logic and styles, consolidating the component to focus solely on the history view. Key changes include updating padding values in MyHistory.module.css, introducing new styles for a team menu with responsive adjustments, and refining existing styles for task lists and detail cards. The component's logic was updated to remove mobile/desktop specific state management, introduce new utility functions for date manipulation, and refine how DoneTask data is fetched and filtered to display tasks by selected month and category. The tasksByDate sorting order was reversed to show newer items first, and a new leftMonthBlocks logic was added to display only months with data. Error handling was improved by adding validation for groupId in API calls and a check before creating comments. Review comments highlighted potential UI issues from removing padding-top on mobile, suggested using CSS variables and extracting common styles for the new team menu, questioned the specificity of global !important chip overrides, and sought confirmation on design alignment for font changes and the new descending sort order for tasks.
|
|
||
| @media (max-width: 1023px) { | ||
| .main { | ||
| padding: 0; |
There was a problem hiding this comment.
The padding-top calculation involving env(safe-area-inset-top) was removed. This might affect the layout on devices with safe areas (e.g., iPhones with notches) in mobile view. Please ensure this removal does not introduce any UI issues on such devices.
| queryFn: async () => { | ||
| // ✅ TODO: 스웨거 경로 확인해서 여기만 맞추면 끝 | ||
| // 흔한 형태 예시: | ||
| // 1) groups/{groupId}/task-lists/{taskListId}/tasks/done?from=...&to=... | ||
| // 2) groups/{groupId}/task-lists/{taskListId}/tasks?done=true&from=...&to=... | ||
| const path = `groups/${groupId}/task-lists/${taskListId}/tasks/done?from=${encodeURIComponent( | ||
| fromIso, | ||
| )}&to=${encodeURIComponent(toIso)}`; | ||
|
|
||
| const data = await apiFetch<DoneTasksResponse>(path); | ||
|
|
||
| // taskListId가 서버 응답에 없으면 주입해서 history에서 필터링 가능하게 | ||
| const tasks = (data?.tasksDone ?? []).map((t) => ({ | ||
| ...t, | ||
| taskListId: (t.taskListId ?? taskListId) as number, | ||
| })); | ||
|
|
||
| return { tasksDone: tasks }; | ||
| if (!Number.isFinite(groupId) || groupId <= 0) { | ||
| throw new ApiError(400, 'Invalid groupId'); | ||
| } | ||
| return await apiFetch<GroupDetailResponse>(`groups/${groupId}`); |
There was a problem hiding this comment.
Adding validation for groupId (!Number.isFinite(groupId) || groupId <= 0) and throwing an ApiError is a critical improvement. This prevents API calls with invalid group IDs, leading to more predictable behavior and better error messages.
| .leftScroll { | ||
| max-height: 768px; | ||
| overflow: auto; | ||
| padding-right: 10px; | ||
| box-sizing: border-box; | ||
| margin-top: 40px; |
There was a problem hiding this comment.
Adding margin-top: 40px; to .leftScroll directly within the style definition is a specific layout adjustment. If this margin is context-dependent, consider if it should be applied conditionally or derived from a spacing system.
| /* TeamHeader 톱니 메뉴 */ | ||
| .teamMenu { | ||
| position: absolute; | ||
| right: 0; | ||
| top: 52px; | ||
| width: 140px; | ||
| background: #fff; | ||
| border: 1px solid #e2e8f0; | ||
| border-radius: 12px; | ||
| box-shadow: 0 10px 24px rgba(15, 23, 42, 0.12); | ||
| z-index: 15000; | ||
| padding: 8px; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 6px; |
There was a problem hiding this comment.
The .teamMenu styling is well-defined. However, consider using CSS variables for colors (#fff, #e2e8f0, #0f172a, #f1f5f9, #ef4444, #fff1f2) to maintain consistency with the rest of the application's theme, as seen in other parts of this file (e.g., var(--color-background-secondary)).
| .teamMenuItem { | ||
| width: 100%; | ||
| height: 34px; | ||
| border: none; | ||
| background: transparent; | ||
| cursor: pointer; | ||
| text-align: center; | ||
| border-radius: 10px; | ||
| font-size: 14px; | ||
| color: #0f172a; | ||
| } |
There was a problem hiding this comment.
The .teamMenuItem and .teamMenuItemDanger classes share many common properties (width, height, border, background, cursor, text-align, border-radius, font-size). These common styles could be extracted into a base class to reduce duplication and improve maintainability.
|
|
||
| const keys = Array.from(map.keys()).sort((a, b) => (a > b ? 1 : -1)); | ||
| return keys.map((k) => ({ dayKey: k, tasks: map.get(k)! })); | ||
| const keys = Array.from(map.keys()).sort((a, b) => (a > b ? -1 : 1)); |
There was a problem hiding this comment.
The sorting order for tasksByDate keys has been changed from ascending (a > b ? 1 : -1) to descending (a > b ? -1 : 1). This means newer dates will appear first. Ensure this change aligns with the desired display order for the history items.
| const list = (map.get(k) ?? []).slice().sort((a, b) => { | ||
| const ai = a.doneAt ?? a.date ?? ''; | ||
| const bi = b.doneAt ?? b.date ?? ''; | ||
| return ai > bi ? -1 : 1; | ||
| }); |
There was a problem hiding this comment.
The inner sorting of tasks within each day (list.sort) has also been changed to descending order based on doneAt or date. This ensures that within each day, the most recently completed tasks appear first.
| if (!effectiveSelectedTaskId) return; | ||
| createComment.mutate({ content }); |
There was a problem hiding this comment.
Adding a check if (!effectiveSelectedTaskId) return; before calling createComment.mutate is a good defensive programming practice. It prevents attempting to create a comment for a non-existent task, improving the robustness of the component.
| flex-direction: column; | ||
| min-width: 0; | ||
| padding: 120px 85px 80px; | ||
| padding: 120px 67px 80px; |
There was a problem hiding this comment.
The padding value changed from 85px to 67px. While this is a valid change, it would be beneficial to understand the reasoning behind this specific value. Is it based on a design system, a new responsive breakpoint, or a visual adjustment?
| const isLoading = queries.some((q) => q.isLoading); | ||
| const isError = queries.some((q) => q.isError); | ||
|
|
||
| const tasksDoneAll = queries.flatMap((q) => (q.data?.tasksDone ?? []) as DoneTask[]); |
There was a problem hiding this comment.
The removal of isError from the return value of useDoneTasksForTaskLists might be an oversight. While isLoading is still present, isError is also a valuable state to expose for comprehensive error handling in the UI. Consider re-adding it if error states need to be explicitly handled.
2 participants
Summary
Issue
Scope