-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat(frontend): landing page ui optimization #149
Conversation
WalkthroughThis pull request introduces several UI and control flow modifications across the frontend. New modal components for sign-in and sign-up have been integrated into the authentication flow with added state management and delayed execution after modal closure. Additionally, routing enhancements have been applied by replacing certain navigational elements with buttons that utilize the router, and minor style adjustments have been made for improved responsiveness and layout. A GraphQL schema file was also tidied by streamlining comment formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant A as AuthChoiceModal
participant M as Modal (SignIn/SignUp)
U->>A: Click "Sign In" or "Create Account" button
A->>A: Trigger onClose to dismiss AuthChoiceModal
A->>A: Start 100ms delay using setTimeout
A->>M: Invoke respective sign in/up function after delay
M-->>U: Display SignIn/SignUp Modal
sequenceDiagram
participant U as User
participant Nav as FloatingNavbar
participant SB as ChatSideBar
participant R as Next Router
U->>Nav: Click "Codefox Journey" tab
Nav-->>U: Alert "Coming Soon."
U->>SB: Click Home button/logo
SB->>R: Execute router.push('/')
R-->>U: Navigate to Home Page
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
frontend/src/app/(main)/page.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/frontend/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. frontend/src/components/root/nav.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/frontend/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. frontend/src/components/root/prompt-form.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/frontend/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
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.
LGTM
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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/components/root/prompt-form.tsx (1)
214-214
: Improved responsive design with flexible widthReplacing the fixed width (
w-[117px]
) withmin-w-max
makes the component adapt to its content size, improving responsiveness and preventing text truncation.For even better responsive behavior across different viewport sizes, consider using:
-'h-6 border-0 focus:ring-0 hover:bg-gray-100 dark:hover:bg-gray-600 pl-1 min-w-max', +'h-6 border-0 focus:ring-0 hover:bg-gray-100 dark:hover:bg-gray-600 pl-1 min-w-max max-w-[200px] md:max-w-none',This would ensure the component doesn't grow too large on small screens while still being flexible.
frontend/src/components/sidebar.tsx (2)
87-120
: Consider refactoring button styling for better maintainabilityThe button styling contains many Tailwind classes that make it hard to maintain. Consider using the
cn()
utility (already imported) to organize styles or extract this into a reusable component.<Button onClick={() => router.push('/')} variant="ghost" - className=" - w-full - h-14 - flex - items-center - justify-start - px-4 - gap-2 - text-sm - xl:text-lg - font-normal - rounded-md - hover:bg-yellow-50 - transition-all - duration-200 - ease-in-out - " + className={cn( + "w-full h-14 flex items-center justify-start px-4 gap-2", + "text-sm xl:text-lg font-normal rounded-md", + "hover:bg-yellow-50 transition-all duration-200 ease-in-out" + )} >
69-74
: Remove console.log statements before productionThere's a console.log statement that should be removed before deploying to production. Consider using a logger that can be toggled based on environment.
- console.log( - 'ChatSideBar state: isCollapsed:', - isCollapsed, - 'currentChatid:', - currentChatid - ); + // Use a logger utility that can be disabled in production + // logger.debug('ChatSideBar state:', { isCollapsed, currentChatid });frontend/src/app/(main)/page.tsx (1)
97-99
: Consider extracting timeout duration as a constantThe timeout duration of 100ms is hardcoded in multiple places. Consider extracting this to a constant for better maintainability.
+ const MODAL_TRANSITION_DELAY = 100; // milliseconds onSignUpClick={() => { setShowAuthChoice(false); setTimeout(() => { setShowSignUp(true); - }, 100); + }, MODAL_TRANSITION_DELAY); }} onSignInClick={() => { setShowAuthChoice(false); setTimeout(() => { setShowSignIn(true); - }, 100); + }, MODAL_TRANSITION_DELAY); }}Also applies to: 103-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/src/app/(main)/page.tsx
(3 hunks)frontend/src/components/auth-choice-modal.tsx
(3 hunks)frontend/src/components/root/nav.tsx
(1 hunks)frontend/src/components/root/prompt-form.tsx
(1 hunks)frontend/src/components/sidebar.tsx
(4 hunks)frontend/src/components/sign-in-modal.tsx
(1 hunks)frontend/src/graphql/schema.gql
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/graphql/schema.gql
🔇 Additional comments (11)
frontend/src/components/sign-in-modal.tsx (1)
46-48
: Good UX enhancement for toast notification positioningAdding position configuration to the toast improves user experience by ensuring notifications appear in a consistent location.
Consider standardizing this position for all toast notifications throughout the application to maintain UI consistency.
frontend/src/components/root/nav.tsx (2)
177-177
: Feature placeholder effectively communicates upcoming functionalityChanging from opening a GitHub link to showing a "Coming Soon" alert clearly communicates to users that this feature is in development.
185-185
:❓ Verification inconclusive
Navigation positioning change impacts layout behavior
Removing the
fixed
class changes the navbar from being fixed at the top of the viewport to scrolling with the page content.This change affects the overall user experience of navigation. Please verify that:
- This was an intentional design change
- The navbar remains accessible throughout the user journey
- It doesn't negatively impact mobile experience
🏁 Script executed:
#!/bin/bash # Check if there are other navigation elements or if this is the primary navigation # Look for other potential navigation elements in the codebase echo "Checking for other navigation components:" rg -l "nav|navbar|navigation" --type tsx --type jsxLength of output: 174
Action Required: Verify Navigation Positioning Change
There is an intentional removal of the
fixed
class from the navbar (infrontend/src/components/root/nav.tsx
at line 185), which changes its behavior from fixed to scrolling with the page content. Please perform the following manual checks:
- Confirm that this design change is intentional.
- Ensure the navbar remains accessible throughout the user journey.
- Validate that the change does not negatively affect the mobile experience.
Run the updated script above to help locate any other navigation components that might indicate alternative or complementary positioning strategies. Then, manually verify that the overall navigation experience is as expected.
🏁 Script executed:
#!/bin/bash echo "Searching for navigation-related components in TSX and JSX files:" rg --type-add "tsx:*.tsx" --type-add "jsx:*.jsx" -l "nav|navbar|navigation"Length of output: 2421
Navigation Positioning Change Verification
The removal of the
fixed
class infrontend/src/components/root/nav.tsx
(line 185) changes the navbar’s behavior from a fixed element at the top to one that scrolls with the content. Based on the search for navigation-related components, no obvious alternative fixed navbar is present that might compensate for this change. However, please manually verify that:
- The design change is intentional.
- The navbar remains visible and accessible throughout the user journey.
- The change does not negatively affect the mobile experience.
frontend/src/components/auth-choice-modal.tsx (2)
41-48
: Improved modal transition with sequential actionsThe implementation now properly separates modal closing from the sign-in action, creating a smoother user experience with the delayed execution.
The pattern of:
- Close current modal
- Wait for animation to complete (100ms)
- Open the next modal
Creates a much more polished user flow.
57-62
: Consistent implementation for sign-up flowThe same improved transition pattern has been correctly applied to the sign-up flow, maintaining consistency.
frontend/src/components/sidebar.tsx (3)
11-11
: Navigation improvements with Next.js RouterGood addition of the
useRouter
hook for client-side navigation which provides a better user experience than traditional page reloads.
52-52
: LGTM - Router instantiationProperly initialized the router for use in navigation actions.
123-123
: Width adjustment improves visual consistencyGood improvement changing from
w-[85%]
tow-full
for the divider, which creates better visual alignment.frontend/src/app/(main)/page.tsx (3)
11-13
: New modal components enhance authentication UXGood addition of separate modals for sign-in and sign-up flows to improve user experience.
18-19
: Good state management for modal visibilityProperly implemented state variables for managing the visibility of authentication modals.
110-111
: Clean implementation of modal componentsThe sign-in and sign-up modals are properly implemented with appropriate props for controlling visibility.
|
||
import { SignInModal } from '@/components/sign-in-modal'; | ||
import { SignUpModal } from '@/components/sign-up-modal'; | ||
import { useRouter } from 'next/navigation'; |
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.
🛠️ Refactor suggestion
Remove unused router import
The useRouter
is imported and instantiated but not used anywhere in the component. You should remove unused imports to keep the code clean.
- import { useRouter } from 'next/navigation';
And:
- const router = useRouter();
Also applies to: 17-17
Summary by CodeRabbit
New Features
Style