Skip to content

Implement modules page#3394

Open
kantord wants to merge 1 commit intomainfrom
implement-modules-page-2
Open

Implement modules page#3394
kantord wants to merge 1 commit intomainfrom
implement-modules-page-2

Conversation

@kantord
Copy link
Copy Markdown
Owner

@kantord kantord commented Aug 1, 2024

No description provided.

@kantord kantord changed the title . Implement modules page Aug 1, 2024
@kantord kantord force-pushed the implement-modules-page-2 branch 3 times, most recently from e5bfab5 to 9ea47e6 Compare August 2, 2024 07:35
@kantord kantord force-pushed the implement-modules-page-2 branch from 9ea47e6 to 7b7a6c1 Compare August 2, 2024 07:39
@kantord kantord marked this pull request as ready for review August 2, 2024 07:40
<Card>
<CardHeader>
<CardTitle>{module.title}</CardTitle>
<CardDescription>Card Description</CardDescription>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<CardDescription>Card Description</CardDescription>
<CardDescription>{module.description}</CardDescription>

}

export default function ModuleCard({ course, module }: Props) {
const coursePageUrl = `/${course.sourceLanguage.code}/courses/${course.targetLanguage.code}/courses/${module.slug}`
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug: Potential Runtime Crash in ModuleCard Component

Issue: The URL building logic in ModuleCard can crash if course or module objects have null/undefined nested properties.

Current problematic code:

const coursePageUrl = `/${course.sourceLanguage.code}/courses/${course.targetLanguage.code}/courses/${module.slug}`

If course or module is null or undefined, then the app might crash 

import { CourseDetail, Module } from '@/data/course'
import Link from 'next/link'

type Props = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: Generic Props type should be renamed to ModuleCardProps for better searchability, IDE support, and maintainability. Follows industry standards and makes refactoring easier.

// ❌ Current
type Props = { course: CourseDetail; module: Module }

// ✅ Suggested  
type ModuleCardProps = { course: CourseDetail; module: Module }

<ul className="flex space-y-6 flex-col p-6">
{detail.modules.map((module) => (
<li key={module.slug}>
<ModuleCard course={detail} module={module} />
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Pass coursePageUrl from parent instead of building it inside the component. Keeps the card as a pure display component without business logic.

// ✅ Better - Pure display component
type ModuleCardProps = {
 course: CourseDetail
 module: Module
 coursePageUrl: string  // Parent handles URL logic
}

@@ -0,0 +1,5 @@
import path from 'node:path'

export function getAbsoluteCoursePath(jsonPath: string) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Use default export since the file contains only a single function.

// ✅ Better - Default export for single function module
export default function getAbsoluteCoursePath(jsonPath: string) {
 return path.join(process.cwd(), 'src', 'courses', jsonPath)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants