-
-
Notifications
You must be signed in to change notification settings - Fork 173
Course landing page #1077
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
Course landing page #1077
Changes from all commits
af4e914
80532a8
fd5aa0e
98e9e3f
f619a8f
dc4d730
b720297
78c0cb8
1b77104
d2e2f81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
|
|
||
| import React from "react"; | ||
| import { Course } from "./type"; | ||
|
|
||
| interface CourseCardProps { | ||
| course: Course; | ||
| onStartCourse: (courseId: number) => void; // Change to number to match your mock data | ||
| } | ||
|
|
||
| const CourseCard: React.FC<CourseCardProps> = ({ course, onStartCourse }) => { | ||
| return ( | ||
| <div | ||
| className={`rounded-lg p-6 shadow-md ${course.featured ? "bg-orange-100 dark:bg-orange-900" : "bg-gray-100 dark:bg-gray-800"}`} | ||
| > | ||
| {/* Course Title */} | ||
| <h2 className="mb-4 text-2xl font-bold">{course.title}</h2> | ||
|
|
||
| {/* Course Description */} | ||
| <p className="mb-4 text-lg text-gray-700 dark:text-gray-300"> | ||
| {course.description} | ||
| </p> | ||
|
|
||
| {/* Course Progress */} | ||
| <div className="flex items-center gap-4"> | ||
| <p className="text-lg">Progress:</p> | ||
| <div className="flex h-16 w-16 items-center justify-center rounded-full bg-pink-500 text-lg font-bold"> | ||
| {course.progress}% | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Start Course Button */} | ||
| <button | ||
| className="mt-4 rounded bg-orange-500 px-4 py-2 text-white hover:bg-orange-600" | ||
| onClick={() => onStartCourse(Number(course.id))} // Call the passed function with course ID | ||
| > | ||
| Start Course | ||
| </button> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| export default CourseCard; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| "use client"; | ||
|
|
||
| import { type Session } from "next-auth"; | ||
| import { notFound } from "next/navigation"; | ||
| import { isFlagEnabled, FEATURE_FLAGS } from "@/utils/flags"; | ||
| import CoursesLanding from "@/components/Course"; | ||
|
|
||
| const Content = ({ session }: { session: Session | null }) => { | ||
| const flagEnabled = isFlagEnabled(FEATURE_FLAGS.COURSES_LANDING); // Adjust to the correct flag | ||
|
|
||
| if (!flagEnabled) { | ||
| notFound(); // Show 404 page if the feature flag is not enabled | ||
| } | ||
|
|
||
| return ( | ||
| <div className="mx-auto max-w-6xl"> | ||
| <CoursesLanding session={session} /> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| export default Content; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,11 +2,11 @@ import Content from "./_client"; | |
| import { getServerAuthSession } from "@/server/auth"; | ||
|
|
||
| export const metadata = { | ||
|
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. Again this should be in the /courses path |
||
| title: "This is a feature flag example", | ||
|
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 whole file shouldn't have been touched. Move your logic over to |
||
| title: "Courses Landing Page", | ||
|
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. Inconsistency between page title and file path The metadata title has been updated to "Courses Landing Page", which aligns with the PR objectives. However, the file is still located in the Consider moving this file to a more appropriate location, such as |
||
| }; | ||
|
|
||
| export default async function Page() { | ||
| // Example of grabbing session in case it is needed | ||
| export default async function CoursesPage() { | ||
| // Get session if needed for authentication purposes | ||
| const session = await getServerAuthSession(); | ||
|
|
||
| return <Content session={session} />; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
|
|
||
| import React from "react"; | ||
| import { useRouter } from "next/router"; | ||
| import { mockCourses } from "./mock"; | ||
| import CourseCard from "./CourseCard"; | ||
| import { type Session } from "next-auth"; | ||
|
|
||
| const CoursesLanding = ({ session }: { session: Session | null }) => { | ||
| const router = useRouter(); // Initialize the useRouter hook | ||
|
|
||
| const handleStartCourse = (courseId: number) => { | ||
| // Navigate to the course content page | ||
| router.push(`/courses/${courseId}`); | ||
|
|
||
| }; | ||
|
|
||
| return ( | ||
| <div className="flex min-h-screen flex-col gap-8 px-4 py-8 dark:bg-gray-900 dark:text-white lg:px-16 lg:py-12"> | ||
| {/* Page Title */} | ||
| <h1 className="mb-6 text-3xl font-bold lg:text-4xl">Courses</h1> | ||
|
|
||
| {/* Courses List with aria-label for accessibility */} | ||
| <div | ||
| className="grid grid-cols-1 gap-8 lg:grid-cols-2" | ||
| aria-label="List of available courses" | ||
| > | ||
| {mockCourses.map((course) => ( | ||
| <CourseCard | ||
| key={course.id} | ||
| course={course} | ||
| onStartCourse={handleStartCourse} | ||
| /> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| export default CoursesLanding; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import { Course } from "./type"; | ||
|
|
||
| // Define the UserProgress type | ||
| type UserProgress = { | ||
| coursesProgress: Array<{ | ||
| courseId: number; // Use string for the ID | ||
| progress: number; // Progress percentage | ||
| featured: boolean; // Include the featured status if needed | ||
| }>; | ||
| }; | ||
|
Comment on lines
+4
to
+10
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. 🛠️ Refactor suggestion Enhance the UserProgress type definition While the
Here's a suggested improvement: type UserProgress = {
coursesProgress: Array<{
courseId: string; // Assuming this will be changed as per previous comment
progress: number & { __brand: 'Percent' }; // Custom type to ensure 0-100 range
featured: boolean;
lastAccessedDate?: Date;
completionDate?: Date;
}>;
};
// Helper function to ensure progress is within 0-100 range
function createPercent(n: number): number & { __brand: 'Percent' } {
if (n < 0 || n > 100) throw new Error('Progress must be between 0 and 100');
return n as number & { __brand: 'Percent' };
}This enhancement improves type safety and future-proofs the type definition. |
||
|
|
||
| // Mock courses data | ||
| export const mockCourses: Course[] = [ | ||
| { | ||
| id: 1, // Changed to string | ||
| title: "Introduction to Web Development", | ||
| description: | ||
| "Learn the basics of HTML, CSS, and JavaScript to build responsive websites.", | ||
| progress: 90, | ||
| featured: true, | ||
| }, | ||
| { | ||
| id: 2, | ||
| title: "Advanced JavaScript Concepts", | ||
| description: | ||
| "Deep dive into JavaScript with a focus on ES6+ features and asynchronous programming.", | ||
| progress: 65, | ||
| featured: false, | ||
| }, | ||
| { | ||
| id: 3, | ||
| title: "React for Beginners", | ||
| description: | ||
| "Understand the fundamentals of React and how to build interactive UIs.", | ||
| progress: 30, | ||
| featured: true, | ||
| }, | ||
| { | ||
| id: 4, | ||
| title: "Full-Stack Web Development", | ||
| description: | ||
| "Learn to build full-stack applications using modern technologies like Node.js and Express.", | ||
| progress: 45, | ||
| featured: false, | ||
| }, | ||
| { | ||
| id: 5, | ||
| title: "Version Control with Git and GitHub", | ||
| description: | ||
| "Master version control using Git and learn how to collaborate on GitHub.", | ||
| progress: 80, | ||
| featured: true, | ||
| }, | ||
| ]; | ||
|
Comment on lines
+13
to
+54
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. 🛠️ Refactor suggestion Refine mockCourses data structure and content
Here's a suggested improvement: export const mockCourses: Course[] = [
{
id: "1",
title: "Introduction to Web Development",
description: "Learn the basics of HTML, CSS, and JavaScript to build responsive websites.",
progress: 90,
featured: true,
},
// ... (other courses with string IDs)
{
id: "6",
title: "Mobile App Development with React Native",
description: "Learn to build cross-platform mobile apps using React Native.",
progress: 0,
featured: false,
},
{
id: "7",
title: "Data Structures and Algorithms",
description: "Master fundamental data structures and algorithms for efficient problem-solving.",
progress: 15,
featured: false,
},
];These changes will improve consistency, cover edge cases, and provide a more diverse dataset for testing. |
||
|
|
||
| // Function to generate user progress | ||
| export const generateUserProgress = (courses: Course[]): UserProgress => ({ | ||
| coursesProgress: courses.map((course) => ({ | ||
| courseId: course.id, // Use the course ID | ||
| progress: course.progress, // Use the course progress | ||
| featured: course.featured, // Include featured status if needed | ||
| })), | ||
| }); | ||
|
Comment on lines
+57
to
+63
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. 🛠️ Refactor suggestion Enhance generateUserProgress function for better type safety and error handling The current implementation of
Here's a suggested improvement: export const generateUserProgress = (courses: Course[]): UserProgress => ({
coursesProgress: courses.map((course) => {
if (typeof course.id !== 'string') {
throw new Error(`Invalid course id: ${course.id}`);
}
if (typeof course.progress !== 'number' || course.progress < 0 || course.progress > 100) {
throw new Error(`Invalid progress for course ${course.id}: ${course.progress}`);
}
return {
courseId: course.id,
progress: course.progress,
featured: course.featured ?? false, // Provide a default value if missing
};
}),
});This implementation adds type checking, error handling, and provides default values where necessary, making the function more robust. |
||
|
|
||
| // Generate the user progress based on mockCourses | ||
| export const userProgress: UserProgress = generateUserProgress(mockCourses); | ||
|
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. 🛠️ Refactor suggestion Enhance userProgress constant definition The current implementation is correct, but we can improve it:
Here's a suggested improvement: /**
* Generates mock user progress data based on the mockCourses.
* This constant is used for development and testing purposes.
* In a real application, this would be fetched from an API or database.
*/
export const getUserProgress = (): UserProgress => generateUserProgress(mockCourses);
// Usage example:
// const userProgress = getUserProgress();This change adds clarity and flexibility to the code, making it easier to understand and potentially reuse in different contexts. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
|
|
||
|
|
||
|
|
||
|
|
||
| export interface Course { | ||
| id: number; | ||
| title: string; | ||
| description?: string; | ||
| progress: number; | ||
|
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. 🛠️ Refactor suggestion Consider using a more specific type for the The You could consider one of these options:
The choice depends on the level of type safety you want to enforce in your codebase. |
||
| featured: boolean; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ import { posthog } from "posthog-js"; | |
|
|
||
| export const FEATURE_FLAGS = { | ||
| FEATURE_FLAG_TEST: "feature-flag-test", | ||
| // Add more feature flags as needed | ||
| COURSES_LANDING: "courses-landing", | ||
|
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 perfect! Thanks |
||
| } as const; | ||
|
|
||
| export type FeatureFlagName = | ||
|
|
@@ -17,11 +17,11 @@ export function isDevEnvironment() { | |
|
|
||
| export const isFlagEnabled = ( | ||
| featureFlag: FeatureFlagName, | ||
| disableDevCheck = false, // Disable dev check to force feature flag to be checked always | ||
| disableDevCheck = false, | ||
|
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. Add back in the comment here as it's a note to developer |
||
| ) => { | ||
| if (!disableDevCheck && isDevEnvironment()) { | ||
| console.log("Feature flag check skipped in development environment"); | ||
| return true; | ||
| return true; // Always true in dev environments unless you want to test differently | ||
| } | ||
| return posthog.isFeatureEnabled(featureFlag); | ||
| }; | ||
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.
This is perfect usage of the feature flag but just in the wrong spot.