Conversation
#254) [REFACTOR] 사진수다 4차 QA 반영
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @jeonbinggu, 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! 이 풀 리퀘스트는 데모데이 이벤트를 위한 전용 페이지를 배포하고, 관련 기능을 추가하는 것을 목표로 합니다. 사용자가 데모 이미지를 통해 AI 사진 복원 기능을 체험하고, 로그인 상태에 따라 적절하게 리다이렉션될 수 있도록 새로운 페이지와 유틸리티가 구현되었습니다. 또한, 기존 페이지들의 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
The pull request introduces a new Demo Day page with animations and functionality for photo restoration, including a redirect mechanism after login. It also includes minor UI adjustments to existing pages, primarily related to button positioning and spacing. The new features appear well-implemented, and the UI changes address potential layout issues on different devices. I've identified a few areas for improvement regarding code clarity and consistency, particularly with magic numbers and animation delays.
| } | ||
|
|
||
| .animate-demoday-fade-in { | ||
| animation: demoday-fade-in 0.6s ease-out both; |
There was a problem hiding this comment.
Consider defining 0.6s as a CSS variable for better maintainability and consistency if this duration is used elsewhere or might change in the future. This makes it easier to update animation durations across the application.
| animation: demoday-fade-in 0.6s ease-out both; | |
| animation: demoday-fade-in var(--demoday-fade-in-duration, 0.6s) ease-out both; |
| } | ||
|
|
||
| .animate-demoday-slide-up { | ||
| animation: demoday-slide-up 0.5s ease-out both; |
There was a problem hiding this comment.
Similar to the fade-in animation, 0.5s could be a CSS variable to centralize animation duration values.
| animation: demoday-slide-up 0.5s ease-out both; | |
| animation: demoday-slide-up var(--demoday-slide-up-duration, 0.5s) ease-out both; |
| } | ||
|
|
||
| .animate-demoday-reveal { | ||
| animation: demoday-reveal 1.8s ease-out both; |
There was a problem hiding this comment.
The 1.8s duration for demoday-reveal could also benefit from being a CSS variable.
| animation: demoday-reveal 1.8s ease-out both; | |
| animation: demoday-reveal var(--demoday-reveal-duration, 1.8s) ease-out both; |
| } | ||
|
|
||
| .animate-demoday-tint { | ||
| animation: demoday-tint 2.4s ease-out both; |
There was a problem hiding this comment.
The 2.4s duration for demoday-tint should also be considered for a CSS variable.
| animation: demoday-tint 2.4s ease-out both; | |
| animation: demoday-tint var(--demoday-tint-duration, 2.4s) ease-out both; |
| onExistingMember: () => { | ||
| // DEMO-DAY: 원래는 navigate("/mainpage", { replace: true }) | ||
| const redirect = consumeRedirectAfterLogin(); | ||
| navigate(redirect ?? "/mainpage", { replace: true }); |
There was a problem hiding this comment.
The comment // DEMO-DAY: 원래는 navigate("/mainpage", { replace: true }) is helpful for understanding the change, but it's generally better to remove such comments after the feature is stable to keep the codebase clean. If the original behavior needs to be referenced, consider documenting it in a more permanent place like a design document or a more general code comment explaining the conditional navigation.
| <div className="flex flex-col gap-4"> | ||
| {/** 게시글 제목 및 내용 */} | ||
| <div className="flex flex-col gap-2"> | ||
| <div className="flex flex-col gap-1"> |
There was a problem hiding this comment.
Changing gap-2 to gap-1 (which is 0.25rem) for the post title and content might reduce the visual separation more than intended. Verify this adjustment against the design.
| className="bg-neutral-875 border-neutral-850 flex flex-col gap-1 rounded-2xl border px-5 py-4 text-left text-neutral-500" | ||
| > | ||
| <div className="gap-2"> | ||
| <div className="flex flex-col gap-1.5"> |
There was a problem hiding this comment.
Changing gap-2 to gap-1.5 (which is 0.375rem) for the lab review section might alter the spacing between elements. Confirm this is the desired layout.
| <div className="flex items-center gap-2"> | ||
| <HomeIcon className="h-4 w-4 font-semibold" /> | ||
| <p className="text-[1rem] font-semibold text-neutral-200"> | ||
| <p className="text-[1rem] font-medium text-neutral-200"> |
There was a problem hiding this comment.
Changing font-semibold to font-medium for the lab name text might make it less prominent. Ensure this change is intentional and matches the desired visual hierarchy.
|
|
||
| return ( | ||
| <div className="mx-auto min-h-dvh w-full max-w-[23.4375rem] pt-[1rem]"> | ||
| <div className="mx-auto min-h-dvh w-full max-w-[23.4375rem]"> |
There was a problem hiding this comment.
Removing pt-[1rem] from the main container might affect the top padding of the page content. If this was intended to remove extra space, ensure it doesn't negatively impact the layout on different screen sizes or when the header is present.
| </div> | ||
|
|
||
| <div className="fixed right-0 bottom-0 left-0 flex justify-center px-5 py-5"> | ||
| <div className="fixed right-0 bottom-0 left-0 flex justify-center px-5 py-5 pb-[calc(1.25rem+env(safe-area-inset-bottom,2.125rem))]"> |
There was a problem hiding this comment.
The pb-[calc(1.25rem+env(safe-area-inset-bottom,2.125rem))] class is a long and complex TailwindCSS class. Consider extracting this into a custom CSS utility class or a Tailwind plugin if it's reused across multiple components. This improves readability and reduces repetition in the JSX.
3 participants
🔀 Pull Request Title
데모데이 전용 페이지 배포
🎞️ 주요 코드 설명
주제1
주제2
📌 PR 설명
이번 PR에서 어떤 작업을 했는지 요약해주세요.
📷 스크린샷
UI 변경이 있을 경우 스크린샷을 첨부해주세요.