Add Radix Accordion + cn helper and publish portfolio with styling refinements#4
Conversation
Summary of ChangesHello @zubinqayam, 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 introduces foundational UI components and a utility function to streamline class name management, alongside deploying a new, fully styled, and interactive personal portfolio page. It also includes important fixes for styling consistency and removes an extraneous script for cleaner deployment. Highlights
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 introduces a reusable Radix-based Accordion component, a cn utility for class names, and a new portfolio page. The changes are well-structured. My review focuses on improving the robustness of the cn utility, enhancing the portfolio page's professionalism and user experience, and adhering to best practices by separating concerns in the page's script. I've identified a couple of high-severity issues related to the cn helper and JavaScript-based styling that should be addressed.
| export function cn(...inputs: ClassValue[]): string { | ||
| let str = ''; | ||
| for (const input of inputs) { | ||
| const val = toVal(input); | ||
| if (val) { | ||
| if (str) str += ' '; | ||
| str += val; | ||
| } | ||
| } | ||
| return str; | ||
| } |
There was a problem hiding this comment.
This cn implementation is a good start for combining class names. However, it doesn't handle merging of conflicting Tailwind CSS classes. For example, cn('p-4', 'p-2') will produce 'p-4 p-2', which can lead to unpredictable styling because both classes are applied. In most Tailwind projects, cn utilities use tailwind-merge to resolve these conflicts, so the output would be just 'p-2'. Since the goal is to avoid external dependencies, be aware of this limitation. You might need to be careful not to pass conflicting utilities, or consider implementing a basic merging logic for common cases (like padding, margin, colors).
| // Add hover effects for experience cards | ||
| document.querySelectorAll('.experience-card').forEach(card => { | ||
| card.addEventListener('mouseenter', function() { | ||
| this.style.transform = 'translateY(-8px) scale(1.02)'; | ||
| }); | ||
|
|
||
| card.addEventListener('mouseleave', function() { | ||
| this.style.transform = 'translateY(0) scale(1)'; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Implementing hover effects via JavaScript by directly manipulating style.transform is not ideal for a few reasons:
- Separation of Concerns: It mixes presentation logic (CSS) with behavior logic (JS). Hover effects are purely presentational and should live in CSS.
- Maintainability: It's harder to manage styles when they are split between a CSS stylesheet and inline styles set by JavaScript.
- Conflicts: This JavaScript code conflicts with the existing CSS
:hoverrule for.experience-cardon line 249. The CSS appliestransform: translateY(-5px), while the JS appliestranslateY(-8px) scale(1.02). This can lead to confusing behavior.
All of these effects can and should be achieved using the :hover pseudo-class in your CSS. I recommend removing this JavaScript block and updating your CSS rule at line 249 to transform: translateY(-8px) scale(1.02);.
| // Add hover effects for experience cards | |
| document.querySelectorAll('.experience-card').forEach(card => { | |
| card.addEventListener('mouseenter', function() { | |
| this.style.transform = 'translateY(-8px) scale(1.02)'; | |
| }); | |
| card.addEventListener('mouseleave', function() { | |
| this.style.transform = 'translateY(0) scale(1)'; | |
| }); | |
| }); | |
| // Hover effects are handled in CSS for better performance and separation of concerns. |
| <h2 class="section-title">Core Competencies</h2> | ||
| <div class="skills-grid"> | ||
| <div class="skill-item"> | ||
| <div class="skill-icon">📊</div> |
There was a problem hiding this comment.
Using emojis for icons is a quick way to add visuals, but they can render inconsistently across different operating systems, browsers, and devices. This can affect the professional look of the portfolio. Additionally, they are not always descriptive for screen reader users. For better consistency and accessibility, I recommend using SVG icons instead. You can embed them directly or use an icon library.
| <div class="skill-icon">📊</div> | |
| <div class="skill-icon"><svg xmlns="http://www.w3.org/2000/svg" width="40" height="40" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><line x1="12" x2="12" y1="20" y2="10"/><line x1="18" x2="18" y1="20" y2="4"/><line x1="6" x2="6" y1="20" y2="16"/></svg></div> |
| document.querySelectorAll('a[href^="#"]').forEach(anchor => { | ||
| anchor.addEventListener('click', function (e) { | ||
| e.preventDefault(); | ||
| const target = document.querySelector(this.getAttribute('href')); | ||
| if (target) { | ||
| target.scrollIntoView({ | ||
| behavior: 'smooth', | ||
| block: 'start' | ||
| }); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The smooth scrolling implementation prevents the default anchor link behavior (e.preventDefault()), which means the URL in the address bar is not updated with the hash (e.g., #about). This breaks deep linking, and users can't copy the URL to share a link to a specific section. Since you already have scroll-behavior: smooth; in your CSS, which is widely supported, you could remove this JavaScript block entirely for a simpler solution that preserves default browser behavior. If you need to support older browsers, consider a polyfill or adjust this script to manually update the URL with history.pushState().
| document.querySelectorAll('a[href^="#"]').forEach(anchor => { | |
| anchor.addEventListener('click', function (e) { | |
| e.preventDefault(); | |
| const target = document.querySelector(this.getAttribute('href')); | |
| if (target) { | |
| target.scrollIntoView({ | |
| behavior: 'smooth', | |
| block: 'start' | |
| }); | |
| } | |
| }); | |
| }); | |
| // Smooth scrolling for navigation links is handled by CSS `scroll-behavior: smooth;`. |
| // Dynamic typing effect for hero title (optional enhancement) | ||
| function typeWriter(element, text, speed = 100) { | ||
| let i = 0; | ||
| element.innerHTML = ''; | ||
| function type() { | ||
| if (i < text.length) { | ||
| element.innerHTML += text.charAt(i); | ||
| i++; | ||
| setTimeout(type, speed); | ||
| } | ||
| } | ||
| type(); | ||
| } | ||
|
|
||
| // Initialize typing effect on page load | ||
| window.addEventListener('load', () => { | ||
| const heroTitle = document.querySelector('.hero-title'); | ||
| const originalText = heroTitle.textContent; | ||
| typeWriter(heroTitle, originalText, 80); | ||
| }); |
There was a problem hiding this comment.
The typewriter effect is a nice touch, but it has a couple of potential issues:
- Animation Conflict: The
.hero-titlealso has afade-inclass, which applies afadeInUpanimation. The typewriter effect, which manipulatesinnerHTML, will run concurrently with the fade-in/slide-up animation, which might not look as intended. - User Experience: The title text is present in the HTML initially, then cleared by the script, and then typed out. This can cause a "flash" of content, which can be jarring. For users on slow networks, there might be a noticeable delay before the typing starts, leaving an empty space.
Consider if this effect is necessary given the fade-in animation is already present. Removing it would provide a cleaner and more predictable user experience.
| // Dynamic typing effect for hero title (optional enhancement) | |
| function typeWriter(element, text, speed = 100) { | |
| let i = 0; | |
| element.innerHTML = ''; | |
| function type() { | |
| if (i < text.length) { | |
| element.innerHTML += text.charAt(i); | |
| i++; | |
| setTimeout(type, speed); | |
| } | |
| } | |
| type(); | |
| } | |
| // Initialize typing effect on page load | |
| window.addEventListener('load', () => { | |
| const heroTitle = document.querySelector('.hero-title'); | |
| const originalText = heroTitle.textContent; | |
| typeWriter(heroTitle, originalText, 80); | |
| }); | |
| // Dynamic typing effect for hero title removed to avoid animation conflicts and content flashing. |
There was a problem hiding this comment.
Pull request overview
This pull request adds a static portfolio landing page for Zubin Qayam, a Radix UI-based accordion component, and a utility function for composing CSS class names. The portfolio page showcases healthcare innovation leadership with responsive design and interactive JavaScript enhancements.
Changes:
- Added a complete static HTML portfolio page with CSS design tokens, responsive styles, and JavaScript animations
- Implemented a
cnutility function inlib/utils.tsfor class name composition - Created reusable Accordion components using Radix UI primitives
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| public/portfolio.html | Full portfolio landing page with embedded styles, responsive layout, and interactive JavaScript features |
| lib/utils.ts | Utility function for composing class names from strings, arrays, and objects |
| components/ui/accordion.tsx | React components wrapping Radix UI accordion primitives with Tailwind styling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| >(({ className, children, ...props }, ref) => ( | ||
| <AccordionPrimitive.Content | ||
| ref={ref} | ||
| className="overflow-hidden text-sm transition-all data-[state=closed]:animate-accordion-up data-[state=open]:animate-accordion-down" |
There was a problem hiding this comment.
The AccordionContent component references animation classes 'animate-accordion-up' and 'animate-accordion-down' that are not defined anywhere in the codebase. These Tailwind CSS animations need to be configured in a tailwind.config file or the animations won't work, resulting in broken accordion expand/collapse effects.
| className="overflow-hidden text-sm transition-all data-[state=closed]:animate-accordion-up data-[state=open]:animate-accordion-down" | |
| className="overflow-hidden text-sm transition-all" |
| /* Responsive Design */ | ||
| @media (max-width: 768px) { | ||
| .nav-links { | ||
| display: none; |
There was a problem hiding this comment.
The navigation links are hidden on mobile devices (max-width: 768px) using display: none, which removes keyboard and screen reader access. Consider using a hamburger menu with proper accessibility features (aria-labels, keyboard navigation) instead of completely hiding navigation on mobile devices.
| display: none; | |
| display: flex; | |
| flex-direction: column; | |
| gap: var(--space-md); |
| <div class="contact-info"> | ||
| <a href="mailto:contact@zubinqayam.com" class="contact-item"> | ||
| <span>📧</span> | ||
| <span>Email</span> | ||
| </a> | ||
| <a href="https://linkedin.com/in/zubinqayam" class="contact-item"> | ||
| <span>💼</span> | ||
| <span>LinkedIn</span> | ||
| </a> | ||
| <a href="tel:+968-XXXXXXXX" class="contact-item"> | ||
| <span>📱</span> | ||
| <span>Phone</span> | ||
| </a> | ||
| </div> |
There was a problem hiding this comment.
Links and buttons lack proper ARIA labels or screen reader text. For example, the contact items use only emoji icons (📧, 💼, 📱) which may not be meaningful to screen reader users. Consider adding aria-label attributes or visually hidden text to describe the purpose of these links.
| // Initialize typing effect on page load | ||
| window.addEventListener('load', () => { | ||
| const heroTitle = document.querySelector('.hero-title'); | ||
| const originalText = heroTitle.textContent; | ||
| typeWriter(heroTitle, originalText, 80); | ||
| }); |
There was a problem hiding this comment.
The dynamic typing effect removes the fade-in class animation that was applied to the hero-title element. The typeWriter function sets innerHTML to empty string, which removes the fade-in class and its animation. This creates a visual inconsistency where the hero subtitle fades in but the title uses the typing effect instead.
| // Add hover effects for experience cards | ||
| document.querySelectorAll('.experience-card').forEach(card => { | ||
| card.addEventListener('mouseenter', function() { | ||
| this.style.transform = 'translateY(-8px) scale(1.02)'; | ||
| }); | ||
|
|
||
| card.addEventListener('mouseleave', function() { | ||
| this.style.transform = 'translateY(0) scale(1)'; | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The hover effects added via JavaScript on lines 594-602 override the CSS transition defined in the experience-card class. The JavaScript directly manipulates the style.transform property, which conflicts with the CSS hover pseudo-class and may cause unexpected behavior or jerky transitions when combined with the CSS-based hover effect.
| // Add hover effects for experience cards | |
| document.querySelectorAll('.experience-card').forEach(card => { | |
| card.addEventListener('mouseenter', function() { | |
| this.style.transform = 'translateY(-8px) scale(1.02)'; | |
| }); | |
| card.addEventListener('mouseleave', function() { | |
| this.style.transform = 'translateY(0) scale(1)'; | |
| }); | |
| }); |
| export type ClassValue = string | number | null | false | undefined | ClassDictionary | ClassArray; | ||
|
|
||
| interface ClassDictionary { | ||
| [key: string]: any; |
There was a problem hiding this comment.
The ClassDictionary interface uses 'any' as the value type, which defeats TypeScript's type safety. The actual logic only checks for truthiness, so the type should be 'any' could be replaced with 'boolean | string | number | null | undefined' or simply 'unknown' to maintain type safety while allowing the truthiness check.
| [key: string]: any; | |
| [key: string]: boolean | string | number | null | undefined; |
| <div class="container"> | ||
| <h2 class="section-title">About</h2> | ||
| <div class="about-content"> | ||
| <div class="about-image"> |
There was a problem hiding this comment.
The hero section buttons and navigation use emoji/icon decorative elements (🏥, 🏭, 🚢, etc.) that should be marked with aria-hidden="true" or role="presentation" to prevent screen readers from announcing them, as they are purely decorative.
| <div class="about-image"> | |
| <div class="about-image" aria-hidden="true"> |
| const AccordionTrigger = React.forwardRef< | ||
| React.ElementRef<typeof AccordionPrimitive.Trigger>, | ||
| React.ComponentPropsWithoutRef<typeof AccordionPrimitive.Trigger> | ||
| >(({ className, children, ...props }, ref) => ( | ||
| <AccordionPrimitive.Header className="flex"> | ||
| <AccordionPrimitive.Trigger | ||
| ref={ref} | ||
| className={cn( | ||
| 'flex flex-1 items-center justify-between py-4 font-medium transition-all hover:underline [&[data-state=open]>svg]:rotate-180', | ||
| className | ||
| )} | ||
| {...props} | ||
| > | ||
| {children} | ||
| <ChevronDown className="h-4 w-4 shrink-0 transition-transform duration-200" /> | ||
| </AccordionPrimitive.Trigger> | ||
| </AccordionPrimitive.Header> | ||
| )); | ||
| AccordionTrigger.displayName = AccordionPrimitive.Trigger.displayName; | ||
|
|
||
| const AccordionContent = React.forwardRef< | ||
| React.ElementRef<typeof AccordionPrimitive.Content>, | ||
| React.ComponentPropsWithoutRef<typeof AccordionPrimitive.Content> | ||
| >(({ className, children, ...props }, ref) => ( | ||
| <AccordionPrimitive.Content | ||
| ref={ref} | ||
| className="overflow-hidden text-sm transition-all data-[state=closed]:animate-accordion-up data-[state=open]:animate-accordion-down" | ||
| {...props} | ||
| > | ||
| <div className={cn('pb-4 pt-0', className)}>{children}</div> | ||
| </AccordionPrimitive.Content> | ||
| )); | ||
|
|
||
| AccordionContent.displayName = AccordionPrimitive.Content.displayName; | ||
|
|
There was a problem hiding this comment.
The AccordionContent component's displayName is set after the component definition but the component is not exported as a named constant before setting displayName. While this works, it's inconsistent with AccordionTrigger which sets displayName using the Primitive's displayName. Consider following the same pattern for consistency.
| const AccordionTrigger = React.forwardRef< | |
| React.ElementRef<typeof AccordionPrimitive.Trigger>, | |
| React.ComponentPropsWithoutRef<typeof AccordionPrimitive.Trigger> | |
| >(({ className, children, ...props }, ref) => ( | |
| <AccordionPrimitive.Header className="flex"> | |
| <AccordionPrimitive.Trigger | |
| ref={ref} | |
| className={cn( | |
| 'flex flex-1 items-center justify-between py-4 font-medium transition-all hover:underline [&[data-state=open]>svg]:rotate-180', | |
| className | |
| )} | |
| {...props} | |
| > | |
| {children} | |
| <ChevronDown className="h-4 w-4 shrink-0 transition-transform duration-200" /> | |
| </AccordionPrimitive.Trigger> | |
| </AccordionPrimitive.Header> | |
| )); | |
| AccordionTrigger.displayName = AccordionPrimitive.Trigger.displayName; | |
| const AccordionContent = React.forwardRef< | |
| React.ElementRef<typeof AccordionPrimitive.Content>, | |
| React.ComponentPropsWithoutRef<typeof AccordionPrimitive.Content> | |
| >(({ className, children, ...props }, ref) => ( | |
| <AccordionPrimitive.Content | |
| ref={ref} | |
| className="overflow-hidden text-sm transition-all data-[state=closed]:animate-accordion-up data-[state=open]:animate-accordion-down" | |
| {...props} | |
| > | |
| <div className={cn('pb-4 pt-0', className)}>{children}</div> | |
| </AccordionPrimitive.Content> | |
| )); | |
| AccordionContent.displayName = AccordionPrimitive.Content.displayName; | |
| const AccordionTrigger = Object.assign( | |
| React.forwardRef< | |
| React.ElementRef<typeof AccordionPrimitive.Trigger>, | |
| React.ComponentPropsWithoutRef<typeof AccordionPrimitive.Trigger> | |
| >(({ className, children, ...props }, ref) => ( | |
| <AccordionPrimitive.Header className="flex"> | |
| <AccordionPrimitive.Trigger | |
| ref={ref} | |
| className={cn( | |
| 'flex flex-1 items-center justify-between py-4 font-medium transition-all hover:underline [&[data-state=open]>svg]:rotate-180', | |
| className | |
| )} | |
| {...props} | |
| > | |
| {children} | |
| <ChevronDown className="h-4 w-4 shrink-0 transition-transform duration-200" /> | |
| </AccordionPrimitive.Trigger> | |
| </AccordionPrimitive.Header> | |
| )), | |
| { displayName: AccordionPrimitive.Trigger.displayName } | |
| ); | |
| const AccordionContent = Object.assign( | |
| React.forwardRef< | |
| React.ElementRef<typeof AccordionPrimitive.Content>, | |
| React.ComponentPropsWithoutRef<typeof AccordionPrimitive.Content> | |
| >(({ className, children, ...props }, ref) => ( | |
| <AccordionPrimitive.Content | |
| ref={ref} | |
| className="overflow-hidden text-sm transition-all data-[state=closed]:animate-accordion-up data-[state=open]:animate-accordion-down" | |
| {...props} | |
| > | |
| <div className={cn('pb-4 pt-0', className)}>{children}</div> | |
| </AccordionPrimitive.Content> | |
| )), | |
| { displayName: AccordionPrimitive.Content.displayName } | |
| ); |
| <p>© 2024 Zubin Qayam. Strategic Healthcare Innovation & Industrial Development Leader.</p> | ||
| </div> | ||
| </footer> | ||
|
|
||
| <script> |
There was a problem hiding this comment.
The copyright year is hardcoded as 2024, but the current year is 2026 according to the context. This should be updated to 2026 or use dynamic JavaScript to display the current year.
| <p>© 2024 Zubin Qayam. Strategic Healthcare Innovation & Industrial Development Leader.</p> | |
| </div> | |
| </footer> | |
| <script> | |
| <p>© <span id="current-year"></span> Zubin Qayam. Strategic Healthcare Innovation & Industrial Development Leader.</p> | |
| </div> | |
| </footer> | |
| <script> | |
| // Set dynamic current year in footer | |
| const currentYearElement = document.getElementById('current-year'); | |
| if (currentYearElement) { | |
| currentYearElement.textContent = new Date().getFullYear(); | |
| } |
| <a href="tel:+968-XXXXXXXX" class="contact-item"> | ||
| <span>📱</span> | ||
| <span>Phone</span> | ||
| </a> |
There was a problem hiding this comment.
The phone contact link uses a placeholder phone number 'tel:+968-XXXXXXXX' which is not functional. This should either be replaced with a real phone number or removed if the actual number is not available yet.
| <a href="tel:+968-XXXXXXXX" class="contact-item"> | |
| <span>📱</span> | |
| <span>Phone</span> | |
| </a> |
1 participant
Motivation
cnhelper.Description
components/ui/accordion.tsximplementingAccordion,AccordionItem,AccordionTrigger, andAccordionContentusing@radix-ui/react-accordionand aChevronDownicon.lib/utils.tswith acn(...inputs)utility for composing class names supporting strings, arrays and objects.public/portfolio.htmlwith full markup, CSS variables, responsive styles, JS for smooth scrolling/fade-in/typing, and later refined palette tokens (--color-slate-*) and the Project Management icon.public/portfolio.htmland adjusted tokens so button styles reference defined variables.Testing
python -m http.serverand ran a Playwright script to loadpublic/portfolio.htmland captureartifacts/portfolio.png, which completed successfully.Codex Task