Conversation
Deploying vntechies-blog with
|
| Latest commit: |
f162d45
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e399adad.blog-68k.pages.dev |
| Branch Preview URL: | https://ui-v3.blog-68k.pages.dev |
There was a problem hiding this comment.
Pull request overview
This is a comprehensive v3 redesign of the VNTechies platform, implementing a modern design system with significant UI/UX improvements across the entire application.
Changes:
- Complete visual redesign with new CSS custom properties-based design system featuring surface panels, improved typography, and consistent spacing
- Refactored homepage with hero section, value propositions, pain points/solutions, learning tracks, testimonials, and enhanced CTAs
- Redesigned course registration flow with dynamic course selection, improved form UX, and visual course previews
- New About page architecture displaying mentors and contributors with stats, replacing the previous MDX-based author layout
- Updated course offerings: Added Data Engineer Bootcamp (VDT-C01), updated DevOps Engineer pricing to 8,000,000 VNĐ, and standardized course naming conventions
Reviewed changes
Copilot reviewed 48 out of 52 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| css/tailwind.css | Introduced comprehensive design system with CSS custom properties for theming, component classes (surface-panel, page-heading, etc.) |
| pages/index.js | Complete homepage redesign with hero, USP pillars, pain points/solutions, learning tracks, testimonials, and CTA sections |
| pages/courses/register.js | Refactored registration with getStaticProps, dynamic course selection, COURSE_META mapping, and improved form UX |
| pages/courses.js | Redesigned course listing with tab-based filtering and new surface-panel styling |
| pages/about.js | Complete refactor from MDX-based to dynamic page showing mentors and contributors with statistics |
| pages/series.js, pages/tags.js | Redesigned with new design system classes and improved layouts |
| pages/career.js | Updated countdown date and styling, uses new design system |
| pages/blog/[...slug].js | Added author slug to authorDetails for profile linking |
| layouts/* | All layouts redesigned with new surface-panel architecture, improved typography, and consistent spacing |
| components/Header.js | Sticky header with improved navigation and responsive design |
| components/Footer.js | Refactored with socialLinks array and improved mobile responsiveness |
| components/CourseRegistrationForm.js | Enhanced with courseLabel prop, improved accessibility (autocomplete, input modes), and course tag display |
| components/home/* | Updated Hero with 5-course grid, added DataCourse component, improved styling |
| data/courses/* | Updated titles to include course codes (VDE-C01, VDT-C01), pricing changes |
| data/authors/* | Updated profiles with mentor flags, student counts, and company information |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| className="absolute right-3 top-3 h-5 w-5 text-gray-400 dark:text-gray-300" | ||
| xmlns="https://www.w3.org/2000/svg" | ||
| className="absolute right-4 top-3.5 h-5 w-5 text-slate-400 dark:text-slate-500" | ||
| xmlns="http://www.w3.org/2000/svg" |
There was a problem hiding this comment.
Inconsistent xmlns attribute. Should be "http://www.w3.org/2000/svg" instead of "https://www.w3.org/2000/svg" (note http vs https). While both may work, the standard SVG namespace uses http.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| )} | ||
| {displayPosts.map((frontMatter) => { | ||
| return ( | ||
| <ArticleList key={frontMatter.title} {...frontMatter} image={frontMatter.images[0]} /> |
There was a problem hiding this comment.
The optional images array should use optional chaining consistently. Using images?.[0] is safer than images[0] when the array might be undefined. Consider using images?.[0] || '/static/images/default-ogp.png' or define a default at the top of the component.
| {prev && ( | ||
| <HorizontalCard | ||
| title={prev.title} | ||
| image={prev.images[0]} | ||
| href={`/blog/${prev.slug}`} | ||
| /> | ||
| )} | ||
| {next && ( | ||
| <HorizontalCard | ||
| title={next.title} | ||
| image={next.images[0]} | ||
| href={`/blog/${next.slug}`} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Potential runtime error when prev.images or next.images are undefined. The code accesses prev.images[0] and next.images[0] without optional chaining. Use prev.images?.[0] and next.images?.[0] instead, or provide a fallback image.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| {(next || prev) && ( | ||
| <div className="mt-8 grid grid-cols-1 gap-4 md:grid-cols-2"> | ||
| {prev && ( | ||
| <HorizontalCard | ||
| title={prev.title} | ||
| image={prev.images[0]} | ||
| href={`/blog/${prev.slug}`} | ||
| /> | ||
| )} | ||
| {(next || prev) && ( | ||
| <div className="my-4 hidden grid-cols-1 gap-4 dark:border-gray-800 md:grid"> | ||
| {prev && ( | ||
| <HorizontalCard | ||
| title={prev.title} | ||
| image={prev.images[0]} | ||
| href={`/blog/${prev.slug}`} | ||
| /> | ||
| )} | ||
| {next && ( | ||
| <HorizontalCard | ||
| title={next.title} | ||
| image={next.images[0]} | ||
| href={`/blog/${next.slug}`} | ||
| /> | ||
| )} | ||
| </div> | ||
| {next && ( | ||
| <HorizontalCard | ||
| title={next.title} | ||
| image={next.images[0]} | ||
| href={`/blog/${next.slug}`} | ||
| /> |
There was a problem hiding this comment.
Potential runtime error when prev.images or next.images are undefined. The code accesses prev.images[0] and next.images[0] without optional chaining. Use prev.images?.[0] and next.images?.[0] instead, or provide a fallback image.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| avatar: /static/images/authors/hanguyen.jpg | ||
| occupation: Thợ Code | ||
| company: Công ty TNNH Một Mình Tui | ||
| avatar: /static/images/authors/hanguyen.jpeg |
There was a problem hiding this comment.
The new avatar image path has changed from .jpg to .jpeg extension. Ensure the file public/static/images/authors/hanguyen.jpeg exists in the repository, otherwise this will result in a broken image.
| avatar: /static/images/authors/hanguyen.jpeg | |
| avatar: /static/images/authors/hanguyen.jpg |
| <label | ||
| htmlFor="isAlumni" | ||
| className={`flex cursor-pointer items-center rounded-lg border border-gray-200 p-4 transition dark:border-gray-600 ${currentTheme.hover}`} | ||
| onClick={() => setFormData({ ...formData, isAlumni: !formData.isAlumni })} | ||
| > | ||
| <input | ||
| id="isAlumni" | ||
| type="checkbox" | ||
| name="isAlumni" | ||
| checked={formData.isAlumni} | ||
| onChange={handleChange} | ||
| className={`mr-3 h-5 w-5 rounded border-gray-300 ${currentTheme.checkbox}`} | ||
| /> | ||
| <label className="cursor-pointer text-sm font-medium text-gray-700 dark:text-gray-300"> | ||
| <span className="text-sm font-medium text-gray-700 dark:text-gray-300"> | ||
| Cựu học viên của VNTechies | ||
| </label> | ||
| </div> | ||
| </span> | ||
| </label> |
There was a problem hiding this comment.
The checkbox inputs are wrapped in label elements but also contain nested label/span elements. This creates invalid HTML as labels cannot be nested. Remove the inner label tags and keep only the span elements, or restructure to avoid nesting labels.
| onClick={() => setTheme(theme === 'dark' || resolvedTheme === 'dark' ? 'light' : 'dark')} | ||
| > | ||
| <svg | ||
| xmlns="https://www.w3.org/2000/svg" |
There was a problem hiding this comment.
Inconsistent xmlns attribute. Should be "http://www.w3.org/2000/svg" instead of "https://www.w3.org/2000/svg" (note http vs https). While both may work, the standard SVG namespace uses http.
| xmlns="https://www.w3.org/2000/svg" | |
| xmlns="http://www.w3.org/2000/svg" |
| }, | ||
| ].map((course, index) => ( | ||
| <Link key={index} href={course.link} className="group"> | ||
| <Link key={index} href={course.link} className="group h-full"> |
There was a problem hiding this comment.
Using array index as key is not recommended in React as it can lead to rendering issues when the array is modified. Since each course has a unique 'code' property, use that as the key instead: key={course.code}.
| <Link key={index} href={course.link} className="group h-full"> | |
| <Link key={course.code} href={course.link} className="group h-full"> |
| useEffect(() => { | ||
| if (!router.isReady || paidCourses.length === 0) return | ||
|
|
||
| const queryValue = Array.isArray(router.query.course) | ||
| ? router.query.course[0] | ||
| : router.query.course | ||
| const normalizedQuery = queryValue?.toString().trim().toLowerCase() | ||
|
|
||
| if (!normalizedQuery) { | ||
| setSelectedSlug(fallbackCourse?.slug || '') | ||
| return | ||
| } | ||
|
|
||
| const matchedCourse = paidCourses.find( | ||
| (course) => | ||
| course.slug.toLowerCase() === normalizedQuery || | ||
| course.code?.toLowerCase() === normalizedQuery | ||
| ) | ||
|
|
||
| if (matchedCourse) setSelectedSlug(matchedCourse.slug) | ||
| }, [router.isReady, router.query.course, paidCourses, fallbackCourse]) |
There was a problem hiding this comment.
The useEffect has an exhaustive dependencies array but fallbackCourse changes reference on every render when paidCourses[0] changes, which could cause unnecessary re-renders. Consider memoizing fallbackCourse or removing it from the dependencies if it's not critical for the effect logic.
No description provided.