-
Notifications
You must be signed in to change notification settings - Fork 44
Improve navigation clarity with active routes and page layout #100
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,9 @@ import { NavLink } from 'react-router-dom' | |
|
|
||
| const Navbar: React.FC = () => { | ||
| 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' | ||
|
Comment on lines
6
to
+9
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The inline type For better type safety and maintainability, you should use the
This will ensure your code is robust against future updates to |
||
|
|
||
| return ( | ||
| <div className="fixed top-2 left-1/2 z-30 flex gap-x-50 items-center justify-between -translate-x-1/2 bg-zinc-900/40 backdrop-blur-md px-5 py-2 rounded-full border border-zinc-500/50 text-white"> | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,24 @@ | ||||||||||||||||||||||||||||||
| import React from "react"; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| type Props = { | ||||||||||||||||||||||||||||||
| title: string; | ||||||||||||||||||||||||||||||
| subtitle?: string; | ||||||||||||||||||||||||||||||
| children: React.ReactNode; | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const PageLayout: React.FC<Props> = ({ title, subtitle, children }) => { | ||||||||||||||||||||||||||||||
|
Comment on lines
+3
to
+9
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a great reusable component! To align it more with modern React best practices and improve clarity, I have a couple of suggestions:
Here's a suggested implementation:
Suggested change
|
||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||
| <div className="pt-24 px-8 max-w-7xl mx-auto"> | ||||||||||||||||||||||||||||||
| <div className="mb-8"> | ||||||||||||||||||||||||||||||
| <h1 className="text-3xl font-bold text-white">{title}</h1> | ||||||||||||||||||||||||||||||
| {subtitle && ( | ||||||||||||||||||||||||||||||
| <p className="text-zinc-400 mt-2">{subtitle}</p> | ||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| {children} | ||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| export default PageLayout; | ||||||||||||||||||||||||||||||
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.
Using
feature.titleas 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
idto each feature object.Example with an
id:This will make your component more robust and ready for real data.