Improve navigation clarity with active routes and page layout #100
Conversation
Summary of ChangesHello @Skbonde05, 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! This pull request significantly improves the user experience by enhancing navigation clarity and providing better page context without altering the existing UI design. It introduces a new 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. 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 improves the user experience by introducing a consistent page layout and clearer navigation. The new PageLayout component is a great addition for standardizing page headers, and the active route highlighting in the navbar makes navigation much more intuitive. I've added a few suggestions to further improve code quality and maintainability, focusing on modern React practices and robustness. Specifically, I've commented on prop typing in Navbar and PageLayout, and on the use of keys in HomePage. Overall, these are excellent changes that enhance the application's structure and usability.
| {features.map((feature) => ( | ||
| <Card key={feature.title} {...feature} /> | ||
| ))} |
There was a problem hiding this comment.
Using feature.title as a key for elements in a list can be problematic if titles are not guaranteed to be unique, which is often the case with dynamic data from an API. This can lead to incorrect rendering and state management issues in React.
A better practice is to use a stable, unique identifier for each item. Since the data is currently mocked, this is a good opportunity to add a unique id to each feature object.
Example with an id:
// In your feature data
{
id: 'custom-models',
title: 'Custom Models',
desc: '...',
}
// In your component
{features.map((feature) => (
<Card key={feature.id} {...feature} />
))}This will make your component more robust and ready for real data.
| const navLinkClass = ({ isActive }: { isActive: boolean }) => | ||
| isActive ? 'text-white line-through opacity-80' : 'text-white' | ||
| isActive | ||
| ? 'text-blue-400 font-semibold underline underline-offset-8' | ||
| : 'text-white hover:text-blue-300 transition-colors' |
There was a problem hiding this comment.
The inline type ({ isActive }: { isActive: boolean }) is not fully correct and can be brittle. The className function on a NavLink receives an object with isActive, isPending, and isTransitioning properties.
For better type safety and maintainability, you should use the NavLinkProps type from react-router-dom.
- Import
NavLinkProps:import { NavLink, type NavLinkProps } from 'react-router-dom'
- Use it to type your function:
const navLinkClass: NavLinkProps['className'] = ({ isActive }) => isActive ? 'text-blue-400 font-semibold underline underline-offset-8' : 'text-white hover:text-blue-300 transition-colors'
This will ensure your code is robust against future updates to react-router-dom.
| type Props = { | ||
| title: string; | ||
| subtitle?: string; | ||
| children: React.ReactNode; | ||
| }; | ||
|
|
||
| const PageLayout: React.FC<Props> = ({ title, subtitle, children }) => { |
There was a problem hiding this comment.
This is a great reusable component! To align it more with modern React best practices and improve clarity, I have a couple of suggestions:
- More Specific Prop Type Name: Rename
PropstoPageLayoutProps. This makes it clearer what the props are for, especially in larger files or when debugging. - Avoid
React.FC: The use ofReact.FCis discouraged in many modern codebases. It has some downsides, like implicitly providingchildren(which you've correctly typed explicitly, making it redundant) and not handling generic props well. A simpler function declaration is preferred.
Here's a suggested implementation:
| type Props = { | |
| title: string; | |
| subtitle?: string; | |
| children: React.ReactNode; | |
| }; | |
| const PageLayout: React.FC<Props> = ({ title, subtitle, children }) => { | |
| type PageLayoutProps = { | |
| title: string; | |
| subtitle?: string; | |
| children: React.ReactNode; | |
| }; | |
| const PageLayout = ({ title, subtitle, children }: PageLayoutProps) => { |
|
@Skbonde05 can you attach screenshot |
|
|
@Skbonde05 why we are getting white bg in left right top and bottom |
|
I will check it |
|
|
@Skbonde05 not looking good |
|
I don't want any padding leave home page as it is if you really want to do something then add some sections |
|
ok |
|
|
@Skbonde05 i already provided you figma link please check once |
|
@Skbonde05 what's the update |
2 participants
This PR enhances overall UX by improving navigation clarity and page context without redesigning the existing UI.
🔹 Persistent Navigation
Utilizes the existing top Navbar across all pages
Ensures consistent visibility during route changes
Keeps navigation intuitive and always accessible
🔹 Active Route Indicators
Highlights the currently active route using color and underline
Makes it immediately clear which section the user is in
Implemented using NavLink active state styling
🔹 Clear Page Context via Page Headers
Introduces a reusable PageLayout component
Each major route now displays:
A clear page title (e.g., Documents & RAG, Model Settings)
A short descriptive subtitle
Helps users understand page purpose at a glance
🔹 Consistent Layout & Spacing
Standardized spacing below the fixed navbar
Consistent placement of page titles and content across sections
Improves visual consistency and reduces cognitive load